Re: [libvirt] [PATCH] storage: Avoid memory leak on metadata fetching

On 07/13/2011 10:56 AM, Michal Privoznik wrote: Existing code had a struct rather than pointer, and you just hoisted the declaration, but this means... 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); 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. 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org