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

[Michal Židek]

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