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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list