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

[Pavel Březina]

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.