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

Michal Židek mzidek at redhat.com
Mon Oct 29 11:41:40 UTC 2012


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sss_cache-Remove-fastcache-even-if-sssd-is-not-runni.patch
Type: text/x-patch
Size: 11571 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121029/d46daf1c/attachment.bin>


More information about the sssd-devel mailing list