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

Simo Sorce simo at redhat.com
Mon Oct 29 14:53:09 UTC 2012


On Mon, 2012-10-29 at 12:53 +0100, Michal Židek wrote:
> On 10/29/2012 12:41 PM, Michal Židek wrote:
> > On 10/26/2012 07:53 PM, Simo Sorce wrote:
> >> 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 ?
> >>
> >
> > That is not necessary, the file will be deleted by sssd_nss. But I added
> > the unlink here in the new patch, so it is more obvious, that the file
> > is no longer needed.
> >
> >>> +    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.
> >>
> >
> > I also changed one debug message in the new patch. In
> > sss_mc_create_file, if we fail to mark the old cache as recycled, we
> > simply continue (failing here would not prevent the processes that use
> > the old cache from using it). I think the message that informs about
> > this failure should be level 0.
> >
> >
> > Thanks for the review.
> >
> > New Patch is attached.
> >
> > NOTE: Just want to remind that this patch applies on top of the [SSSD]
> > [PATCH] sss_cache: Multiple domains not handled properly
> >
> > Michal
> >
> 
> There was bad indentation on the place where I changed the debug level 
> of the message.
> 
> 
> New patch attached.

Codewise looks ok, but I still see duplication of the code used to lock
the file.

I was wondering, would it make sense to split this patch in 2 and put
the lock_mc_file functions as a more generic function in the common
utils as a separate patch first ?

I would see it added in util/util_lock.c so it is available to both
tools and responders (avoids the duplication you still have).

The prototype should be something like:

static errno_t sss_br_lock_file(int fd, size_t start, size_t len, int
retries, int wait);

Where wait is expressed in milliseconds.

Also you do not really need to unlock the file if you are going to
close() it.
Posix semantics require the OS to drop all locks on a file if you close
it, so you can safely drop the logic around unlocking in tools_util.c
(however add a comment before the close like: /* closing the file will
drop the lock */ ).

Simo.

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




More information about the sssd-devel mailing list