[SSSD] [PATCH] mmap_cache: Store corrupted mmap cache before reset

Michal Židek mzidek at redhat.com
Wed Aug 21 23:20:26 UTC 2013


On 08/21/2013 04:47 PM, Michal Židek wrote:
> On 08/21/2013 02:01 PM, Jakub Hrozek wrote:
>> On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
>>> On (21/08/13 09:50), Pavel Březina wrote:
>>>> On 08/19/2013 09:56 PM, Michal Židek wrote:
>>>>> +static void  sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx)
>>>>> +{
>>>>> +    int err;
>>>>> +    int fd = -1;
>>>>> +    ssize_t written;
>>>>> +    char *file = NULL;
>>>>> +    TALLOC_CTX *tmp_ctx;
>>>>> +
>>>>> +    if (mc_ctx == NULL) {
>>>>> +        DEBUG(SSSDBG_TRACE_FUNC,
>>>>> +              ("Cannot store uninitialized cache. Nothing to
>>>>> do.\n"));
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    tmp_ctx = talloc_new(NULL);
>>>>> +    if (tmp_ctx == NULL) {
>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    file = talloc_asprintf(tmp_ctx, "%s_%s",
>>>>> +                           mc_ctx->file, "corrupted");
>>>>> +    if (file == NULL) {
>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));
>>>>> +        goto done;
>>>>> +    }
>>>>> +
>>>>> +    /* We will always store only the last problematic cache state */
>>>>> +    fd = creat(file, 0600);
>>>>> +    if (fd == -1) {
>>>>> +        err = errno;
>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
>>>>> +              ("Failed to open file '%s' [%d]: %s\n",
>>>>> +               file, err, strerror(err)));
>>>>> +        goto done;
>>>>> +    }
>>>>> +
>>>>> +    written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
>>>>
>>>> Shouldn't sss_atomic_write_s() be used?
>
> sss_atomic_write_s is probably better. I though that the function
> is only useful for non-blocking sockets, but after reading it's code I
> see it is good to use it here too.
>
>>>>> +    if (written != mc_ctx->mmap_size) {
>>>>> +        if (written == -1) {
>>>>> +            err = errno;
>>>>> +            DEBUG(SSSDBG_CRIT_FAILURE,
>>>>> +                  ("write() failed [%d]: %s\n", err, strerror(err)));
>>>>> +        } else {
>>>>> +            DEBUG(SSSDBG_CRIT_FAILURE,
>>>>> +                  ("write() returned %zd (expected (%zd))\n",
>>>>> +                   written, mc_ctx->mmap_size));
>>>>> +        }
>>>>> +        goto done;
>>>>> +    }
>>>>
>>>> I think the file should be unlinked in case of error.
>
> Ok. I added unlink() call for the case if write returns -1. If the
> file was only partially written (for whatever reason) it can still
> hold some useful information, so I would prefer to keep it.
>
>>>>
>>>>> +
>>>>> +    sss_log(SSS_LOG_NOTICE,
>>>>> +            "Stored copy of corrupted mmap cache in file '%s\n'",
>>>>> file);
>>>>> +done:
>>>>> +    if (fd != -1) {
>>>>> +        close(fd);
>>>>> +    }
>>>>> +    talloc_free(tmp_ctx);
>>>>> +}
>>>
>>> I think that function sss_mc_save_corrupted will never be called with
>>> patches
>>> from thread "[PATCHES] mmap_cache: Skip records which doesn't have
>>> same hash"
>>>
>>> It is very likely that it is a dead code. I would prefer to ignore this
>>> nitpicks, because patches have already been pushed.
>>>
>>> LS
>>
>> No, sorry, I would prefer to always fix and improve code. At the very
>> least, existing code acts as a reference when developing new code.
>
> Since the original patch was already pushed, this one just adds the
> requested changes.
>
> Thanks
> Michal
>

Since this was not yet pushed I did one more cosmetic change. I think
calling the close() before unlink() is more readable. If the fd != -1 we 
already know that we want to close it, so the conditional check for
unlink() just makes noise before it. And this order is probably more
natural for most cases.

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mmap_cache-Use-sss_atomic_write_s-instead-of-write.patch
Type: text/x-patch
Size: 1766 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130822/d1a33d84/attachment.bin>


More information about the sssd-devel mailing list