[SSSD] [PATCH] sss_cache: Remove fastcache even if sssd is not running.

Simo Sorce simo at redhat.com
Fri Oct 26 17:53:55 UTC 2012


On Fri, 2012-10-26 at 18:33 +0200, Michal Židek wrote:
> --- a/src/responder/nss/nsssrv_mmap_cache.c
> +++ b/src/responder/nss/nsssrv_mmap_cache.c
> @@ -537,9 +537,46 @@ static errno_t sss_mc_create_file(struct
> sss_mc_ctx *mc_ctx)
>      mode_t old_mask;
>      int ofd;
>      int ret;
> +    int retries_left;
> +    struct flock mc_lock;
> +    useconds_t t = 50000;
> +
> +    mc_lock.l_type = F_WRLCK;
> +    mc_lock.l_whence = SEEK_SET;
> +    mc_lock.l_start = 0;
> +    mc_lock.l_start = 1;

Something wrong here ^ this should be l_len

> +    mc_lock.l_pid = getpid();

You do not need to set this, it's a read-out variable, and should be
initialized to 0.

>      ofd = open(mc_ctx->file, O_RDWR);
>      if (ofd != -1) {
> +        for (retries_left = 2; retries_left >= 0; retries_left--) {

nitpick but this would be more readable as:
   for (retries_left = 3; retries_left > 0; retries_left--) {
however I see it would require (retries_left - 1) in the debug message,
so if you prefer the current form add a comment that says /* try 3 times
*/ before the for loop.


> @@ -570,6 +607,34 @@ static errno_t sss_mc_create_file(struct
> sss_mc_ctx *mc_ctx)
>                                      mc_ctx->file, ret,
> strerror(ret)));
>      }
>  
> +    for (retries_left = 2; retries_left >= 0; retries_left--) {
> +        ret = fcntl(mc_ctx->fd, F_SETLK, &mc_lock);
> +        if (ret == EACCES || ret == EAGAIN || ret == EINTR) {
> +            /* File is locked by someone else */
> +            DEBUG(SSSDBG_TRACE_FUNC,
> +                  ("Failed to lock mc file %s. Retries left: %d\n",
> +                   mc_ctx->file, retries_left));
> +            ret = usleep(t);
> +            if (ret == -1) {
> +                DEBUG(SSSDBG_MINOR_FAILURE,
> +                      ("usleep() failed -> ignoring\n"));
> +            }
> +        } else if (ret == 0) {
> +            /* File successfuly locked */
> +            break;
> +        } else {
> +            /* Error occurred */
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  ("Unable to lock mc file %s\n", mc_ctx->file));
> +            return ret;
> +        }
> +    }
> +    if (retries_left < 0) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              ("Unable to lock mc file %s\n", mc_ctx->file));
> +        return ret;
> +    }
> +

This block is the same as above, please do not repeat blocks of code,
create a static helper function instead and use it in both places.

> +errno_t sss_memcache_invalidate(const char *mc_filename)
> +{
> +    int mc_fd = -1;
> +    struct flock mc_lock;
> +    errno_t ret;
> +    bool locked = false;
> +
> +    if (!mc_filename) {
> +        return EINVAL;
> +    }
> +
> +    mc_fd = open(mc_filename, O_RDWR);
> +    if (mc_fd == -1) {
> +        ret = errno;
> +        if (ret == ENOENT) {
> +            DEBUG(SSSDBG_TRACE_FUNC,("Memory cache file %s "
> +                  "does not exist.\n", mc_filename));
> +            return EOK;
> +        } else {
> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to open file %s: %s
> \n",
> +                  mc_filename, strerror(ret)));
> +            return ret;
> +        }
> +    }
> +
> +    mc_lock.l_type = F_WRLCK;
> +    mc_lock.l_whence = SEEK_SET;
> +    mc_lock.l_start = 0;
> +    mc_lock.l_start = 1;
> +    mc_lock.l_pid = getpid();

Same comments as per sssd_nss code.

> +    ret = fcntl(mc_fd, F_SETLK, &mc_lock);
> +    if (ret == -1) {
> +        ret = errno;
> +        if (ret == EACCES || ret == EAGAIN || ret == EINTR) {
> +            /* File already locked by sssd_nss */
> +            ret = EACCES;
> +            goto done;

EINTR must be handled differently here, I would retry at least once on
EINTR as it doesn't mean the file is locked, only that the syscall was
interrupted before the lock could be attempted.
(The server case is different because it makes no difference for what
reason you fail, there you have to retry anyway, here only for EINTR).

> +        } else {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  ("Failed to lock memory cache file %s: %s\n",
> +                   mc_filename, strerror(ret)));
> +            goto done;
> +        }
> +    }
> +    locked = true;
> +
> +    /* Mark the mc file as recycled. */
> +    ret = sss_mc_set_recycled(mc_fd);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to mark memory cache file
> %s "
> +              "as recycled.\n", mc_filename));
> +        goto done;
> +    }
> +
> +    ret = EOK;
> +done:
> +    if (locked) {
> +        mc_lock.l_type = F_UNLCK;
> +        fcntl(mc_fd, F_SETLK, &mc_lock);
> +    }
> +
> +    if (mc_fd != -1) {
> +        close(mc_fd);
> +    }

Shouldn't you also unlink the file here ?

> +    return ret;
> +}

In general the approach is correct and the patch is ok, please fix the
bugs and will get in master right away.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list