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

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


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