[PATCH 0/7] Various fixes
Fixes for stuff I've noticed when reviewing Tim's hash table cleanups. This obviously applies on top of his series. Peter Krempa (7): virHashNew: Ensure return value is used hypervCreateEmbeddedParam: Don't count elements needlessly qemuBlockNodeNameGetBackingChain: Shuffle around variable initialization virDomainDefValidateAliases: Refactor to avoid cleanup section virNetworkObjNew: Remove impossible error handling qemu: domain: Move qemuDomainObjPrivateAlloc after qemuDomainObjPrivateFree qemuDomainObjPrivateAlloc: Fix unlikely memory leak src/conf/domain_validate.c | 15 ++++------ src/conf/virnetworkobj.c | 13 +++------ src/hyperv/hyperv_wmi.c | 5 ---- src/qemu/qemu_block.c | 8 ++---- src/qemu/qemu_domain.c | 57 ++++++++++++++++++-------------------- src/util/virhash.h | 2 +- 6 files changed, 39 insertions(+), 61 deletions(-) -- 2.31.1
Declare the function with G_GNUC_WARN_UNUSED_RESULT as we always want to use the returned value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhash.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virhash.h b/src/util/virhash.h index c2a0681317..426d835cfc 100644 --- a/src/util/virhash.h +++ b/src/util/virhash.h @@ -50,7 +50,7 @@ typedef int (*virHashSearcher) (const void *payload, const char *name, /* * Constructor and destructor. */ -GHashTable *virHashNew(virHashDataFree dataFree); +GHashTable *virHashNew(virHashDataFree dataFree) G_GNUC_WARN_UNUSED_RESULT; virHashAtomic *virHashAtomicNew(virHashDataFree dataFree); void virHashFree(GHashTable *table); ssize_t virHashSize(GHashTable *table); -- 2.31.1
'count' is not used after calculating it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/hyperv/hyperv_wmi.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index dc2c6471ab..e211fd5d80 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -274,16 +274,11 @@ GHashTable * hypervCreateEmbeddedParam(hypervWmiClassInfo *classInfo) { size_t i; - size_t count; g_autoptr(GHashTable) table = virHashNew(NULL); XmlSerializerInfo *typeinfo = NULL; typeinfo = classInfo->serializerInfo; - /* loop through the items to find out how many fields there are */ - for (count = 0; typeinfo[count].name != NULL; count++) - ; - for (i = 0; typeinfo[i].name != NULL; i++) { XmlSerializerInfo *item = &typeinfo[i]; -- 2.31.1
Allocate the hash tables first so tat the 'data' struct can be directly initialized removing the need for a memset and two additional assignments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 8150241015..1bcf7fa4f1 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -230,20 +230,16 @@ GHashTable * qemuBlockNodeNameGetBackingChain(virJSONValue *namednodes, virJSONValue *blockstats) { - struct qemuBlockNodeNameGetBackingChainData data; g_autoptr(GHashTable) namednodestable = virHashNew(virJSONValueHashFree); g_autoptr(GHashTable) disks = virHashNew(qemuBlockNodeNameBackingChainDataHashEntryFree); - - memset(&data, 0, sizeof(data)); + struct qemuBlockNodeNameGetBackingChainData data = { .nodenamestable = namednodestable, + .disks = disks }; if (virJSONValueArrayForeachSteal(namednodes, qemuBlockNamedNodesArrayToHash, namednodestable) < 0) return NULL; - data.nodenamestable = namednodestable; - data.disks = disks; - if (virJSONValueArrayForeachSteal(blockstats, qemuBlockNodeNameGetBackingChainDisk, &data) < 0) -- 2.31.1
Use a temporary auto-freed local variable to hold the hash table so that the cleanup section can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index aab377fbbd..a9e4519b1a 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -1257,25 +1257,20 @@ static int virDomainDefValidateAliases(const virDomainDef *def, GHashTable **aliases) { - struct virDomainDefValidateAliasesData data; - int ret = -1; - /* We are not storing copies of aliases. Don't free them. */ - data.aliases = virHashNew(NULL); + g_autoptr(GHashTable) tmpaliases = virHashNew(NULL); + struct virDomainDefValidateAliasesData data = { .aliases = tmpaliases }; if (virDomainDeviceInfoIterateFlags((virDomainDef *) def, virDomainDeviceDefValidateAliasesIterator, DOMAIN_DEVICE_ITERATE_ALL_CONSOLES, &data) < 0) - goto cleanup; + return -1; if (aliases) - *aliases = g_steal_pointer(&data.aliases); + *aliases = g_steal_pointer(&tmpaliases); - ret = 0; - cleanup: - virHashFree(data.aliases); - return ret; + return 0; } -- 2.31.1
'obj->classIdMap' is a bitmap with size of '16', thus the first 3 bits are guaranteed to be available. Use 'virBitmapSetBit' instead of 'virBitmapSetBitExpand' since we don't need any expansion and ignore errors as they are impossible. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virnetworkobj.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index ea021892c7..2a2d94dc98 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -112,21 +112,16 @@ virNetworkObjNew(void) obj->classIdMap = virBitmapNew(INIT_CLASS_ID_BITMAP_SIZE); - /* The first three class IDs are already taken */ - if (virBitmapSetBitExpand(obj->classIdMap, 0) < 0 || - virBitmapSetBitExpand(obj->classIdMap, 1) < 0 || - virBitmapSetBitExpand(obj->classIdMap, 2) < 0) - goto error; + /* The first three class IDs are already taken. */ + ignore_value(virBitmapSetBit(obj->classIdMap, 0)); + ignore_value(virBitmapSetBit(obj->classIdMap, 1)); + ignore_value(virBitmapSetBit(obj->classIdMap, 2)); obj->ports = virHashNew(virNetworkObjPortFree); virObjectLock(obj); return obj; - - error: - virObjectUnref(obj); - return NULL; } -- 2.31.1
The freeing function will be needed to undo failures in allocation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 61 +++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b919da6eab..8d1ecc0140 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1772,36 +1772,6 @@ qemuDomainObjStopWorker(virDomainObj *dom) } -static void * -qemuDomainObjPrivateAlloc(void *opaque) -{ - qemuDomainObjPrivate *priv; - - priv = g_new0(qemuDomainObjPrivate, 1); - - if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks) < 0) { - virReportSystemError(errno, "%s", - _("Unable to init qemu driver mutexes")); - goto error; - } - - if (!(priv->devs = virChrdevAlloc())) - goto error; - - priv->blockjobs = virHashNew(virObjectFreeHashData); - - /* agent commands block by default, user can choose different behavior */ - priv->agentTimeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK; - priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; - priv->driver = opaque; - - return priv; - - error: - VIR_FREE(priv); - return NULL; -} - /** * qemuDomainObjPrivateDataClear: * @priv: domain private data @@ -1922,6 +1892,37 @@ qemuDomainObjPrivateFree(void *data) } +static void * +qemuDomainObjPrivateAlloc(void *opaque) +{ + qemuDomainObjPrivate *priv; + + priv = g_new0(qemuDomainObjPrivate, 1); + + if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks) < 0) { + virReportSystemError(errno, "%s", + _("Unable to init qemu driver mutexes")); + goto error; + } + + if (!(priv->devs = virChrdevAlloc())) + goto error; + + priv->blockjobs = virHashNew(virObjectFreeHashData); + + /* agent commands block by default, user can choose different behavior */ + priv->agentTimeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK; + priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; + priv->driver = opaque; + + return priv; + + error: + VIR_FREE(priv); + return NULL; +} + + static int qemuStorageSourcePrivateDataAssignSecinfo(qemuDomainSecretInfo **secinfo, char **alias) -- 2.31.1
Additional cleanup paths add the possibility of not freeing earlier stuff. Add an AUTOPTR handler for qemuDomainObjPrivate and use it in qemuDomainObjPrivateAlloc Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8d1ecc0140..7e2efc8168 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1891,22 +1891,22 @@ qemuDomainObjPrivateFree(void *data) g_free(priv); } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainObjPrivate, qemuDomainObjPrivateFree); + static void * qemuDomainObjPrivateAlloc(void *opaque) { - qemuDomainObjPrivate *priv; - - priv = g_new0(qemuDomainObjPrivate, 1); + g_autoptr(qemuDomainObjPrivate) priv = g_new0(qemuDomainObjPrivate, 1); if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks) < 0) { virReportSystemError(errno, "%s", _("Unable to init qemu driver mutexes")); - goto error; + return NULL; } if (!(priv->devs = virChrdevAlloc())) - goto error; + return NULL; priv->blockjobs = virHashNew(virObjectFreeHashData); @@ -1915,11 +1915,7 @@ qemuDomainObjPrivateAlloc(void *opaque) priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; priv->driver = opaque; - return priv; - - error: - VIR_FREE(priv); - return NULL; + return g_steal_pointer(&priv); } -- 2.31.1
On Fri, 2021-07-23 at 11:05 +0200, Peter Krempa wrote:
Fixes for stuff I've noticed when reviewing Tim's hash table cleanups.
This obviously applies on top of his series.
Peter Krempa (7): virHashNew: Ensure return value is used hypervCreateEmbeddedParam: Don't count elements needlessly qemuBlockNodeNameGetBackingChain: Shuffle around variable initialization virDomainDefValidateAliases: Refactor to avoid cleanup section virNetworkObjNew: Remove impossible error handling qemu: domain: Move qemuDomainObjPrivateAlloc after qemuDomainObjPrivateFree qemuDomainObjPrivateAlloc: Fix unlikely memory leak
src/conf/domain_validate.c | 15 ++++------ src/conf/virnetworkobj.c | 13 +++------ src/hyperv/hyperv_wmi.c | 5 ---- src/qemu/qemu_block.c | 8 ++---- src/qemu/qemu_domain.c | 57 ++++++++++++++++++------------------ -- src/util/virhash.h | 2 +- 6 files changed, 39 insertions(+), 61 deletions(-)
LGTM. Series: Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
participants (2)
-
Peter Krempa -
Tim Wiederhake