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

Lukas Slebodnik lslebodn at redhat.com
Wed Aug 21 12:22:03 UTC 2013


On (21/08/13 14:01), 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?
>> >
>> >>+    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.
>> >
>> >>+
>> >>+    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.
sensible argument.
In this case, I agree too.

LS



More information about the sssd-devel mailing list