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

[Lukas Slebodnik]

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