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

Michal Židek mzidek at redhat.com
Mon Oct 29 11:53:24 UTC 2012


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.

Thanks
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: 11739 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121029/252e3231/attachment.bin>


More information about the sssd-devel mailing list