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