[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