Re: [libvirt] [PATCH] cgroup: avoid leaking a file

[Laine Stump <laine laine org>]

On 05/04/2011 03:35 AM, Daniel Veillard wrote:
> On Wed, May 04, 2011 at 12:30:07AM -0400, Laine Stump wrote:
> > On 05/03/2011 05:49 PM, Eric Blake wrote:
> > > Clang detected a dead store to rc.  It turns out that in fixing this,
> > > I also found a FILE* leak.
> > >
> > > * src/util/cgroup.c (virCgroupKillInternal): Abort rather than
> > > resuming loop on fscanf failure, and cleanup file on error.
> > This definitely fixes the FILE* leak, but do we really want to abort
> > the loop (stop looking for more pidfiles) when fscanf fails on one
> > pidfile? (dunno, just asking)
> [...]
> > > @@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr
> > >                       if (feof(fp))
> > >                           break;
> > >                       rc = -errno;
> > > -                    break;
> > > +                    VIR_DEBUG("Failed to read %s: %m\n", keypath);
> > > +                    goto cleanup;
> > >                   }
> > >                   if (virHashLookup(pids, (void*)pid))
> > >                       continue;
>    Seems to me we were doing a break anyway, so it should not change
> behaviour there,


The break will get you out of the inner while (!feof(fp)) loop, but if 
you managed to get through that inner loop once before failing, and set 
done = false, you will then loop again through while (!done) and go to 
the next file (if one exists). If you change the break into "goto 
cleanup", you will skip any remaining files.

I don't know if that's important or not, but it is a change in behavior.

> ACK, but let's have a second patch for after 0.9.1
> if we think this need improvement.
>
> Daniel