[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] qemuDomainSnapshotLoad leak



On 04/07/2010 11:47 AM, Jim Meyering wrote:
> clang reports that the assignment to "snap" below is a dead store.
> Actually it's also a leak, and it seems like it deserves a diagnostic
> if/when that function returns NULL.

It is a dead store, that is true, but it's not a leak.  The pointer that
is returned there is a pointer to the internal snapshot object, which
we want to keep around.  I guess we can just do:

virDomainSnapshotAssignDef(&vm->snapshots, def);

And don't do the assignment at all.  And yes, we should also check for
NULL and print an error if it fails.

> 
> It looks to me like this function should return
> something other than "void", so that it can inform
> its caller of such a failure.
> 
> No?

...but no, we should leave this function returning void.  If something (anything)
fails in here, there is nothing a higher level can do.  We don't want to fail
to start libvirtd because of one corrupt/bogus snapshot metadata file,
so the most we can do is print an error message and go on.

-- 
Chris Lalancette


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]