On Thu, Jul 14, 2011 at 12:56:26 +0200, Michal Privoznik wrote:
> Getting metadata on storage allocates a memory (path) which need to
> be freed after use otherwise it gets leaked. This means after use of
> virStorageFileGetMetadataFromFD or virStorageFileGetMetadata one
> must call virStorageFileFreeMetadata to free it. This function frees
> structure internals and structure itself.
> ---
> diff to v1:
> -Eric's review suggestions taken in
>
> src/conf/domain_conf.c | 17 ++++++++---
> src/libvirt_private.syms | 1 +
> src/storage/storage_backend_fs.c | 53 +++++++++++++++++++++----------------
> src/util/storage_file.c | 16 +++++++++++
> src/util/storage_file.h | 2 +
> 5 files changed, 61 insertions(+), 28 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 39e59b9..c89c7c6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11055,6 +11055,12 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> int ret = -1;
> size_t depth = 0;
> char *nextpath = NULL;
> + virStorageFileMetadata *meta;
> +
> + if (VIR_ALLOC(meta) < 0) {
> + virReportOOMError();
> + return ret;
> + }
>
> if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
> return 0;
You leak the allocated meta here. Let's move the allocation code after this
second if statement.
> @@ -11084,7 +11090,6 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> paths = virHashCreate(5, NULL);
>
> do {
> - virStorageFileMetadata meta;
> const char *path = nextpath ? nextpath : disk->src;
> int fd;
>
> @@ -11112,7 +11117,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> }
> }
>
> - if (virStorageFileGetMetadataFromFD(path, fd, format, &meta) < 0) {
> + if (virStorageFileGetMetadataFromFD(path, fd, format, meta) < 0) {
> VIR_FORCE_CLOSE(fd);
> goto cleanup;
> }
> @@ -11126,16 +11131,17 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> goto cleanup;
>
> depth++;
> - nextpath = meta.backingStore;
> + VIR_FREE(nextpath);
> + nextpath = meta->backingStore;
You need to set meta->backingStore = NULL here, otherwise...
>
> /* Stop iterating if we reach a non-file backing store */
> - if (nextpath && !meta.backingStoreIsFile) {
> + if (nextpath && !meta->backingStoreIsFile) {
> VIR_DEBUG("Stopping iteration on non-file backing store: %s",
> nextpath);
> break;
> }
>
> - format = meta.backingStoreFormat;
> + format = meta->backingStoreFormat;
>
> if (format == VIR_STORAGE_FILE_AUTO &&
> !allowProbing)
> @@ -11151,6 +11157,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> cleanup:
> virHashFree(paths);
> VIR_FREE(nextpath);
> + virStorageFileFreeMetadata(meta);
... the memory block referenced from meta->backingStore may be freed twice
here, once in nextpath and once in meta->backingStore.
>
> return ret;
> }
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 3237d18..e06c7fc 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -940,6 +940,7 @@ virStorageGenerateQcowPassphrase;
> # storage_file.h
> virStorageFileFormatTypeFromString;
> virStorageFileFormatTypeToString;
> +virStorageFileFreeMetadata;
> virStorageFileGetMetadata;
> virStorageFileGetMetadataFromFD;
> virStorageFileIsSharedFS;
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index b30e01e..821efb7 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -61,8 +61,13 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
> unsigned long long *capacity,
> virStorageEncryptionPtr *encryption)
> {
> - int fd, ret;
> - virStorageFileMetadata meta;
> + int fd = -1, ret = -1;
Personally, I prefer
int fd = -1;
int ret = -1;
> + virStorageFileMetadata *meta;
> +
> + if (VIR_ALLOC(meta) < 0) {
> + virReportOOMError();
> + return ret;
> + }
>
> *backingStore = NULL;
> *backingStoreFormat = VIR_STORAGE_FILE_AUTO;
> @@ -71,36 +76,33 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
>
> if ((ret = virStorageBackendVolOpenCheckMode(target->path,
> VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0)
> - return ret; /* Take care to propagate ret, it is not always -1 */
> + goto error; /* Take care to propagate ret, it is not always -1 */
> fd = ret;
>
> if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd,
> allocation,
> capacity)) < 0) {
> - VIR_FORCE_CLOSE(fd);
> - return ret;
> + goto error;
> }
>
> - memset(&meta, 0, sizeof(meta));
> -
> if ((target->format = virStorageFileProbeFormatFromFD(target->path, fd)) < 0) {
> - VIR_FORCE_CLOSE(fd);
> - return -1;
> + ret = 1;
Looks like a typo here, it should be
ret = -1;
> + goto error;
> }
>
> if (virStorageFileGetMetadataFromFD(target->path, fd,
> target->format,
> - &meta) < 0) {
> - VIR_FORCE_CLOSE(fd);
> - return -1;
> + meta) < 0) {
> + ret = -1;
> + goto error;
> }
>
> VIR_FORCE_CLOSE(fd);
>
> - if (meta.backingStore) {
> - *backingStore = meta.backingStore;
> - meta.backingStore = NULL;
> - if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) {
> + if (meta->backingStore) {
> + *backingStore = meta->backingStore;
> + meta->backingStore = NULL;
> + if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO) {
> if ((ret = virStorageFileProbeFormat(*backingStore)) < 0) {
> /* If the backing file is currently unavailable, only log an error,
> * but continue. Returning -1 here would disable the whole storage
> @@ -114,18 +116,17 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
> ret = 0;
> }
> } else {
> - *backingStoreFormat = meta.backingStoreFormat;
> + *backingStoreFormat = meta->backingStoreFormat;
> ret = 0;
> }
> } else {
> - VIR_FREE(meta.backingStore);
> ret = 0;
> }
>
> - if (capacity && meta.capacity)
> - *capacity = meta.capacity;
> + if (capacity && meta->capacity)
> + *capacity = meta->capacity;
>
> - if (encryption != NULL && meta.encrypted) {
> + if (encryption != NULL && meta->encrypted) {
> if (VIR_ALLOC(*encryption) < 0) {
> virReportOOMError();
> goto cleanup;
> @@ -147,11 +148,17 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
> */
> }
>
> + virStorageFileFreeMetadata(meta);
> +
> return ret;
>
> +error:
> + VIR_FORCE_CLOSE(fd);
> +
> cleanup:
> - VIR_FREE(*backingStore);
> - return -1;
> + virStorageFileFreeMetadata(meta);
> + return ret;
> +
To avoid code duplication, this could have been
...
cleanup:
virStorageFileFreeMetadata(meta);
return ret;
error:
VIR_FORCE_CLOSE(fd);
goto cleanup;
But it's just one line, so not a big deal (unless the cleanup code is
expanded, in which case it's easier to forgot to update both chunks).
...
> @@ -912,6 +916,18 @@ virStorageFileGetMetadata(const char *path,
> return ret;
> }
>
> +/**
> + * virStorageFileFreeMetadata:
> + *
> + * Free pointers in passed structure, but not memory used by
> + * structure itself.
> + */
> +void ATTRIBUTE_NONNULL(1)
> +virStorageFileFreeMetadata(virStorageFileMetadata *meta)
> +{
> + VIR_FREE(meta->backingStore);
> + VIR_FREE(meta);
> +}
We like having free-like functions to work with NULL arguments, shouldn't we
follow that habit here as well?
> diff --git a/src/util/storage_file.h b/src/util/storage_file.h
> index f1bdd02..b362796 100644
> --- a/src/util/storage_file.h
> +++ b/src/util/storage_file.h
> @@ -70,6 +70,8 @@ int virStorageFileGetMetadataFromFD(const char *path,
> int format,
> virStorageFileMetadata *meta);
>
> +void virStorageFileFreeMetadata(virStorageFileMetadata meta);
> +
On the other hand, if we insist on having ATTRIBUTE_NONNULL, we should say
that in the header file.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list