[PATCH 0/6] util: alloc: Remove VIR_ALLOC_VAR
One user was reimplemented using a more common approach, two users use a constant size so were reimplemented using g_malloc + sizeof. Peter Krempa (6): virNWFilterVarCombIter: Allocate 'iter' member separately virLockManagerSanlockAddLease: Refactor cleanup virLockManagerSanlockAddDisk: Refactor cleanup locking: sanlock: Avoid use of VIR_ALLOC_VAR for 'struct sanlk_resource' virNetDevGetEthtoolGFeatures: Avoid use of VIR_ALLOC_VAR viralloc: Remove VIR_ALLOC_VAR src/conf/nwfilter_params.c | 6 ++-- src/conf/nwfilter_params.h | 2 +- src/libvirt_private.syms | 1 - src/locking/lock_driver_sanlock.c | 50 +++++++++++-------------------- src/util/viralloc.c | 33 -------------------- src/util/viralloc.h | 35 ---------------------- src/util/virnetdev.c | 5 ++-- 7 files changed, 24 insertions(+), 108 deletions(-) -- 2.29.2
Switch to the more common approach of having arrays allocated separately rather than trailing the struct. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_params.c | 6 ++++-- src/conf/nwfilter_params.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 18b64e373b..1be492759a 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -304,6 +304,8 @@ virNWFilterVarCombIterFree(virNWFilterVarCombIterPtr ci) for (i = 0; i < ci->nIter; i++) g_free(ci->iter[i].varNames); + g_free(ci->iter); + g_free(ci); } @@ -465,8 +467,8 @@ virNWFilterVarCombIterCreate(GHashTable *hash, int iterIndex = -1; unsigned int nextIntIterId = VIR_NWFILTER_MAX_ITERID + 1; - if (VIR_ALLOC_VAR(res, virNWFilterVarCombIterEntry, 1 + nVarAccess) < 0) - return NULL; + res = g_new0(virNWFilterVarCombIter, 1); + res->iter = g_new0(virNWFilterVarCombIterEntry, nVarAccess + 1); res->hashTable = hash; diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h index 05a6a43399..59068b8ae9 100644 --- a/src/conf/nwfilter_params.h +++ b/src/conf/nwfilter_params.h @@ -137,7 +137,7 @@ typedef virNWFilterVarCombIter *virNWFilterVarCombIterPtr; struct _virNWFilterVarCombIter { GHashTable *hashTable; size_t nIter; - virNWFilterVarCombIterEntry iter[0]; + virNWFilterVarCombIterEntryPtr iter; }; virNWFilterVarCombIterPtr virNWFilterVarCombIterCreate( GHashTable *hash, -- 2.29.2
Use g_autofree and remove the 'cleanup' section and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/locking/lock_driver_sanlock.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 9c38a1d2f6..77c8d9542b 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -528,12 +528,11 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock, bool shared) { virLockManagerSanlockPrivatePtr priv = lock->privateData; - int ret = -1; - struct sanlk_resource *res = NULL; + g_autofree struct sanlk_resource *res = NULL; size_t i; if (VIR_ALLOC_VAR(res, struct sanlk_disk, 1) < 0) - goto cleanup; + return -1; res->flags = shared ? SANLK_RES_SHARED : 0; res->num_disks = 1; @@ -541,7 +540,7 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock, virReportError(VIR_ERR_INTERNAL_ERROR, _("Resource name '%s' exceeds %d characters"), name, SANLK_NAME_LEN); - goto cleanup; + return -1; } for (i = 0; i < nparams; i++) { @@ -550,7 +549,7 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock, virReportError(VIR_ERR_INTERNAL_ERROR, _("Lease path '%s' exceeds %d characters"), params[i].value.str, SANLK_PATH_LEN); - goto cleanup; + return -1; } } else if (STREQ(params[i].key, "offset")) { res->disks[0].offset = params[i].value.ul; @@ -559,20 +558,15 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock, virReportError(VIR_ERR_INTERNAL_ERROR, _("Resource lockspace '%s' exceeds %d characters"), params[i].value.str, SANLK_NAME_LEN); - goto cleanup; + return -1; } } } - priv->res_args[priv->res_count] = res; + priv->res_args[priv->res_count] = g_steal_pointer(&res); priv->res_count++; - ret = 0; - - cleanup: - if (ret == -1) - VIR_FREE(res); - return ret; + return 0; } -- 2.29.2
Use g_autofree to allow removal of 'cleanup:' and the 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/locking/lock_driver_sanlock.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 77c8d9542b..f35a0c065c 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -581,10 +581,9 @@ virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, bool shared) { virLockManagerSanlockPrivatePtr priv = lock->privateData; - int ret = -1; - struct sanlk_resource *res = NULL; - char *path = NULL; - char *hash = NULL; + g_autofree struct sanlk_resource *res = NULL; + g_autofree char *path = NULL; + g_autofree char *hash = NULL; if (nparams) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -593,17 +592,17 @@ virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, } if (VIR_ALLOC_VAR(res, struct sanlk_disk, 1) < 0) - goto cleanup; + return -1; res->flags = shared ? SANLK_RES_SHARED : 0; res->num_disks = 1; if (virCryptoHashString(VIR_CRYPTO_HASH_MD5, name, &hash) < 0) - goto cleanup; + return -1; if (virStrcpy(res->name, hash, SANLK_NAME_LEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("MD5 hash '%s' unexpectedly larger than %d characters"), hash, (SANLK_NAME_LEN - 1)); - goto cleanup; + return -1; } path = g_strdup_printf("%s/%s", driver->autoDiskLeasePath, res->name); @@ -611,7 +610,7 @@ virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, virReportError(VIR_ERR_INTERNAL_ERROR, _("Lease path '%s' exceeds %d characters"), path, SANLK_PATH_LEN); - goto cleanup; + return -1; } if (virStrcpy(res->lockspace_name, @@ -620,20 +619,13 @@ virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, virReportError(VIR_ERR_INTERNAL_ERROR, _("Resource lockspace '%s' exceeds %d characters"), VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); - goto cleanup; + return -1; } - priv->res_args[priv->res_count] = res; + priv->res_args[priv->res_count] = g_steal_pointer(&res); priv->res_count++; - ret = 0; - - cleanup: - if (ret == -1) - VIR_FREE(res); - VIR_FREE(path); - VIR_FREE(hash); - return ret; + return 0; } -- 2.29.2
In both cases we need memory for a 'struct sanlk_resource' followed by one 'struct sanlk_disk', thus there's no risk of overflow. Use g_malloc0 and sizeof() to allocate the memory instead of VIR_ALLOC_VAR. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/locking/lock_driver_sanlock.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index f35a0c065c..7319f56819 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -531,8 +531,7 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock, g_autofree struct sanlk_resource *res = NULL; size_t i; - if (VIR_ALLOC_VAR(res, struct sanlk_disk, 1) < 0) - return -1; + res = g_malloc0(sizeof(struct sanlk_resource) + sizeof(struct sanlk_disk)); res->flags = shared ? SANLK_RES_SHARED : 0; res->num_disks = 1; @@ -591,8 +590,7 @@ virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver, return -1; } - if (VIR_ALLOC_VAR(res, struct sanlk_disk, 1) < 0) - return -1; + res = g_malloc0(sizeof(struct sanlk_resource) + sizeof(struct sanlk_disk)); res->flags = shared ? SANLK_RES_SHARED : 0; res->num_disks = 1; -- 2.29.2
In this case we need a 'struct ethtool_gfeatures' followed by two 'struct ethtool_get_features_block' so there's no risk of overflow. Use g_malloc0 and sizeof() to allocate the memory instead of VIR_ALLOC_VAR. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virnetdev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1ef7cea20a..6ee59989ed 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3300,9 +3300,8 @@ virNetDevGetEthtoolGFeatures(const char *ifname, { g_autofree struct ethtool_gfeatures *g_cmd = NULL; - if (VIR_ALLOC_VAR(g_cmd, - struct ethtool_get_features_block, GFEATURES_SIZE) < 0) - return -1; + g_cmd = g_malloc0(sizeof(struct ethtool_gfeatures) + + sizeof(struct ethtool_get_features_block) * GFEATURES_SIZE); g_cmd->cmd = ETHTOOL_GFEATURES; g_cmd->size = GFEATURES_SIZE; -- 2.29.2
The use case VIR_ALLOC_VAR deals with is very unlikely. We had just 2 legitimate uses, which were reimplemented locally using g_malloc0 and sizeof instead as they used a static number of members of the trailing array. Remove VIR_ALLOC_VAR since in most cases the direct implementation is shorter and clearer and there are no users of it currently. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/viralloc.c | 33 --------------------------------- src/util/viralloc.h | 35 ----------------------------------- 3 files changed, 69 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 30589c08ac..0636b0d8c9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1724,7 +1724,6 @@ vir_g_strdup_vprintf; # util/viralloc.h -virAllocVar; virDeleteElementsN; virExpandN; virInsertElementsN; diff --git a/src/util/viralloc.c b/src/util/viralloc.c index e4dc13b776..1317537c8a 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -260,36 +260,3 @@ virDeleteElementsN(void *ptrptr, size_t size, size_t at, virShrinkN(ptrptr, size, countptr, toremove); return 0; } - -/** - * virAllocVar: - * @ptrptr: pointer to hold address of allocated memory - * @struct_size: size of initial struct - * @element_size: size of array elements - * @count: number of array elements to allocate - * - * Allocate struct_size bytes plus an array of 'count' elements, each - * of size element_size. This sort of allocation is useful for - * receiving the data of certain ioctls and other APIs which return a - * struct in which the last element is an array of undefined length. - * The caller of this type of API is expected to know the length of - * the array that will be returned and allocate a suitable buffer to - * contain the returned data. C99 refers to these variable length - * objects as structs containing flexible array members. - * - * Returns -1 on failure, 0 on success - */ -int virAllocVar(void *ptrptr, - size_t struct_size, - size_t element_size, - size_t count) -{ - size_t alloc_size = 0; - - if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) - abort(); - - alloc_size = struct_size + (element_size * count); - *(void **)ptrptr = g_malloc0(alloc_size); - return 0; -} diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 29e3224818..e3027622c4 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -49,8 +49,6 @@ int virInsertElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, size_t toremove, bool inPlace) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); -int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count) - G_GNUC_WARN_UNUSED_RESULT ATTRIBUTE_NONNULL(1); /** * VIR_REALLOC_N: @@ -292,39 +290,6 @@ int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t co #define VIR_DELETE_ELEMENT_INPLACE(ptr, at, count) \ virDeleteElementsN(&(ptr), sizeof(*(ptr)), at, &(count), 1, true) -/** - * VIR_ALLOC_VAR_OVERSIZED: - * @M: size of base structure - * @N: number of array elements in trailing array - * @S: size of trailing array elements - * - * Check to make sure that the requested allocation will not cause - * arithmetic overflow in the allocation size. - */ -#define VIR_ALLOC_VAR_OVERSIZED(M, N, S) ((((size_t)-1) - (M)) / (S) < (N)) - -/** - * VIR_ALLOC_VAR: - * @ptr: pointer to hold address of allocated memory - * @type: element type of trailing array - * @count: number of array elements to allocate - * - * Allocate sizeof(*ptr) bytes plus an array of 'count' elements, each - * sizeof('type'). This sort of allocation is useful for receiving - * the data of certain ioctls and other APIs which return a struct in - * which the last element is an array of undefined length. The caller - * of this type of API is expected to know the length of the array - * that will be returned and allocate a suitable buffer to contain the - * returned data. C99 refers to these variable length objects as - * structs containing flexible array members. - * - * This macro is safe to use on arguments with side effects. - * - * Returns 0 on success, aborts on OOM - */ -#define VIR_ALLOC_VAR(ptr, type, count) \ - virAllocVar(&(ptr), sizeof(*(ptr)), sizeof(type), (count)) - /** * VIR_FREE: * @ptr: pointer holding address to be freed -- 2.29.2
On Wed, Feb 03, 2021 at 02:34:20PM +0100, Peter Krempa wrote:
One user was reimplemented using a more common approach, two users use a constant size so were reimplemented using g_malloc + sizeof.
Peter Krempa (6): virNWFilterVarCombIter: Allocate 'iter' member separately virLockManagerSanlockAddLease: Refactor cleanup virLockManagerSanlockAddDisk: Refactor cleanup locking: sanlock: Avoid use of VIR_ALLOC_VAR for 'struct sanlk_resource' virNetDevGetEthtoolGFeatures: Avoid use of VIR_ALLOC_VAR viralloc: Remove VIR_ALLOC_VAR
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Peter Krempa