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

Jakub Hrozek jhrozek at redhat.com
Thu Aug 22 17:48:04 UTC 2013


On Thu, Aug 22, 2013 at 10:45:40AM +0200, Pavel Březina wrote:
> On 08/22/2013 01:20 AM, Michal Židek wrote:
> >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
> 
> Ack.

Pushed to master.



More information about the sssd-devel mailing list