[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