On 07/13/2011 10:56 AM, Michal Privoznik wrote:
+++ b/src/conf/domain_conf.c @@ -11055,6 +11055,9 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, int ret = -1; size_t depth = 0; char *nextpath = NULL; + virStorageFileMetadata meta;
Existing code had a struct rather than pointer, and you just hoisted the declaration, but this means...
+ + memset(&meta, 0, sizeof(virStorageFileMetadata));
you had to manually clear it, but I guess I can live with that. On the other hand, it looks like our prevailing style has instead been to use: virStorageFileMetadata *meta; if (VIR_ALLOC(meta) < 0) report OOM use meta virStorageFileMetadataFree(meta);
@@ -11137,6 +11140,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
format = meta.backingStoreFormat;
+ virStorageFileFreeMetadata(meta);
Here, you are passing by copy rather than by reference, so while this frees the pointer to plug the leak, it leaves the caller's copy of meta.backingStore now as a stale pointer. Not very nice. You have to pass by reference so that the caller will see backingStore as NULL after the cleanup function.
+++ b/src/libvirt_private.syms @@ -940,6 +940,7 @@ virStorageGenerateQcowPassphrase; # storage_file.h virStorageFileFormatTypeFromString; virStorageFileFormatTypeToString; +virStorageFileFreeMetadata;
Generally, functions with Free in the name should be free-like (operate on a pointer and free it, rather than operating on a struct by copy), and listed in cfg.mk if they are no-ops when called on a NULL pointer. Meanwhile, we use the name Clear for functions that operate on a struct's members but do not free the struct itself (see domain_conf.c for lots of examples). If you decide not to convert existing uses from an in-place struct to a malloc'd pointer, then what you are implementing might look better as: void ATTRIBUTE_NONNULL(1) virStorageFileMetadataClear(virStorageFileMetadata *meta) { VIR_FREE(meta->backingStore); } and update callers to do virStorageFileMetadataClear(&meta). I agree that there's a leak to be plugged, but think we need a v2 that clears up the naming and uses pass-by-reference, as well as deciding whether to convert over to malloc'd pointers rather than on-stack structs. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org