[libvirt PATCH 00/10] virHashNew refactorings - part VI
"virHashNew" cannot return NULL, yet we check for NULL in various places. See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html. Tim Wiederhake (10): qemuStateInitialize: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: Use automatic memory management qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return NULL qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory management qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s testQemuGetLatestCaps: `virHashNew` cannot return NULL testQemuGetLatestCaps: Use automatic memory management testQemuGetLatestCaps: Remove superfluous `goto`s src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_monitor.c | 13 +++---------- src/qemu/qemu_monitor_json.c | 26 ++++++++------------------ tests/testutilsqemu.c | 13 +++---------- 4 files changed, 15 insertions(+), 40 deletions(-) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df44c3fbd0..521063d438 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -753,8 +753,7 @@ qemuStateInitialize(bool privileged, if (!(qemu_driver->hostdevMgr = virHostdevManagerGetDefault())) goto error; - if (!(qemu_driver->sharedDevices = virHashNew(qemuSharedDeviceEntryFree))) - goto error; + qemu_driver->sharedDevices = virHashNew(qemuSharedDeviceEntryFree); if (qemuMigrationDstErrorInit(qemu_driver) < 0) goto error; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f66a3457c1..940eeab9a8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4522,8 +4522,7 @@ qemuMonitorGetPRManagerInfo(qemuMonitor *mon, QEMU_CHECK_MONITOR(mon); - if (!(info = virHashNew(g_free))) - goto cleanup; + info = virHashNew(g_free); if (qemuMonitorJSONGetPRManagerInfo(mon, info) < 0) goto cleanup; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 940eeab9a8..36b1f15c7d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4516,21 +4516,18 @@ qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo) { int ret = -1; - GHashTable *info = NULL; + g_autoptr(GHashTable) info = virHashNew(g_free); *retinfo = NULL; QEMU_CHECK_MONITOR(mon); - info = virHashNew(g_free); - if (qemuMonitorJSONGetPRManagerInfo(mon, info) < 0) goto cleanup; *retinfo = g_steal_pointer(&info); ret = 0; cleanup: - virHashFree(info); return ret; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 36b1f15c7d..b06c842a7a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4515,7 +4515,6 @@ int qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo) { - int ret = -1; g_autoptr(GHashTable) info = virHashNew(g_free); *retinfo = NULL; @@ -4523,12 +4522,10 @@ qemuMonitorGetPRManagerInfo(qemuMonitor *mon, QEMU_CHECK_MONITOR(mon); if (qemuMonitorJSONGetPRManagerInfo(mon, info) < 0) - goto cleanup; + return -1; *retinfo = g_steal_pointer(&info); - ret = 0; - cleanup: - return ret; + return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor_json.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 223777739d..472308e1d3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5089,8 +5089,7 @@ qemuMonitorJSONGetAllBlockJobInfo(qemuMonitor *mon, } nr_results = virJSONValueArraySize(data); - if (!(blockJobs = virHashNew(g_free))) - goto cleanup; + blockJobs = virHashNew(g_free); for (i = 0; i < nr_results; i++) { virJSONValue *entry = virJSONValueArrayGet(data, i); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor_json.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 472308e1d3..59c5eedb0a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5069,12 +5069,12 @@ GHashTable * qemuMonitorJSONGetAllBlockJobInfo(qemuMonitor *mon, bool rawjobname) { - virJSONValue *cmd = NULL; - virJSONValue *reply = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; virJSONValue *data; size_t nr_results; size_t i; - GHashTable *blockJobs = NULL; + g_autoptr(GHashTable) blockJobs = NULL; cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL); if (!cmd) @@ -5103,14 +5103,10 @@ qemuMonitorJSONGetAllBlockJobInfo(qemuMonitor *mon, } cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return blockJobs; + return g_steal_pointer(&blockJobs); error: - virHashFree(blockJobs); - blockJobs = NULL; - goto cleanup; + return NULL; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor_json.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 59c5eedb0a..7e53e0ce84 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5074,39 +5074,34 @@ qemuMonitorJSONGetAllBlockJobInfo(qemuMonitor *mon, virJSONValue *data; size_t nr_results; size_t i; - g_autoptr(GHashTable) blockJobs = NULL; + g_autoptr(GHashTable) blockJobs = virHashNew(g_free); cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL); if (!cmd) return NULL; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return NULL; if ((data = virJSONValueObjectGetArray(reply, "return")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("reply was missing return data")); - goto cleanup; + return NULL; } nr_results = virJSONValueArraySize(data); - blockJobs = virHashNew(g_free); for (i = 0; i < nr_results; i++) { virJSONValue *entry = virJSONValueArrayGet(data, i); if (!entry) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing array element")); - goto error; + return NULL; } if (qemuMonitorJSONParseBlockJobInfo(blockJobs, entry, rawjobname) < 0) - goto error; + return NULL; } - cleanup: return g_steal_pointer(&blockJobs); - - error: - return NULL; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/testutilsqemu.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index be576037e2..72c0b1857b 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -585,12 +585,9 @@ testQemuGetLatestCaps(void) "s390x", "x86_64", }; - GHashTable *capslatest; + GHashTable *capslatest = virHashNew(g_free); size_t i; - if (!(capslatest = virHashNew(g_free))) - goto error; - VIR_TEST_VERBOSE(""); for (i = 0; i < G_N_ELEMENTS(archs); ++i) { -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/testutilsqemu.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 72c0b1857b..72b53096d9 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -585,7 +585,7 @@ testQemuGetLatestCaps(void) "s390x", "x86_64", }; - GHashTable *capslatest = virHashNew(g_free); + g_autoptr(GHashTable) capslatest = virHashNew(g_free); size_t i; VIR_TEST_VERBOSE(""); @@ -601,10 +601,9 @@ testQemuGetLatestCaps(void) VIR_TEST_VERBOSE(""); - return capslatest; + return g_steal_pointer(&capslatest); error: - virHashFree(capslatest); return NULL; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/testutilsqemu.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 72b53096d9..fab676c070 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -594,7 +594,7 @@ testQemuGetLatestCaps(void) char *cap = testQemuGetLatestCapsForArch(archs[i], "xml"); if (!cap || virHashAddEntry(capslatest, archs[i], cap) < 0) - goto error; + return NULL; VIR_TEST_VERBOSE("latest caps for %s: %s", archs[i], cap); } @@ -602,9 +602,6 @@ testQemuGetLatestCaps(void) VIR_TEST_VERBOSE(""); return g_steal_pointer(&capslatest); - - error: - return NULL; } -- 2.31.1
On Mon, Jul 19, 2021 at 11:55:44 +0200, Tim Wiederhake wrote:
"virHashNew" cannot return NULL, yet we check for NULL in various places.
See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html.
Tim Wiederhake (10): qemuStateInitialize: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: Use automatic memory management qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return NULL qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory management qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s testQemuGetLatestCaps: `virHashNew` cannot return NULL testQemuGetLatestCaps: Use automatic memory management testQemuGetLatestCaps: Remove superfluous `goto`s
Series: Reviewed-by: Peter Krempa <pkrempa@redhat.com> For any further cleanups please consider fixing similar instances (e.g. removal of NULL check for virHashNew) on a per-file basis rather than per function. Same way in most cases where you switching to automatic memory freeing on a per-function basis it's okay to include cleanup of jumps/labels. We mostly madated separation if you want to do it on a per-file basis.
On Mon, 2021-07-19 at 14:08 +0200, Peter Krempa wrote:
On Mon, Jul 19, 2021 at 11:55:44 +0200, Tim Wiederhake wrote:
"virHashNew" cannot return NULL, yet we check for NULL in various places.
See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html .
Tim Wiederhake (10): qemuStateInitialize: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: Use automatic memory management qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return NULL qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory management qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s testQemuGetLatestCaps: `virHashNew` cannot return NULL testQemuGetLatestCaps: Use automatic memory management testQemuGetLatestCaps: Remove superfluous `goto`s
Series:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
For any further cleanups please consider fixing similar instances (e.g. removal of NULL check for virHashNew) on a per-file basis rather than per function.
Same way in most cases where you switching to automatic memory freeing on a per-function basis it's okay to include cleanup of jumps/labels.
We mostly madated separation if you want to do it on a per-file basis.
I have 38 patches still in my queue. 6 of those are two "cannot return null; use g_auto*; remove goto" sequences that I would like to keep separate. The remaining patches are all of the "cannot return null" kind and spread over the entire code base. Applying them on a per-file basis would reduce the number of patches by only four, from 32 to 28. I am aware that this refactoring is quite noisy, but I do not think that grouping by file makes it significantly better. What I can offer is grouping by directory / subsystem (e.g. conf, nwfilter, qemu, util, etc.), or sending out all remaining patches in one go, so that the amount of mails is the same, but the process is hopefully faster. Your thoughts? Tim
On Mon, Jul 19, 2021 at 14:36:59 +0200, Tim Wiederhake wrote:
On Mon, 2021-07-19 at 14:08 +0200, Peter Krempa wrote:
On Mon, Jul 19, 2021 at 11:55:44 +0200, Tim Wiederhake wrote:
"virHashNew" cannot return NULL, yet we check for NULL in various places.
See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html .
Tim Wiederhake (10): qemuStateInitialize: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL qemuMonitorGetPRManagerInfo: Use automatic memory management qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return NULL qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory management qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s testQemuGetLatestCaps: `virHashNew` cannot return NULL testQemuGetLatestCaps: Use automatic memory management testQemuGetLatestCaps: Remove superfluous `goto`s
Series:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
For any further cleanups please consider fixing similar instances (e.g. removal of NULL check for virHashNew) on a per-file basis rather than per function.
Same way in most cases where you switching to automatic memory freeing on a per-function basis it's okay to include cleanup of jumps/labels.
We mostly madated separation if you want to do it on a per-file basis.
I have 38 patches still in my queue.
6 of those are two "cannot return null; use g_auto*; remove goto" sequences that I would like to keep separate. The remaining patches are all of the "cannot return null" kind and spread over the entire code base. Applying them on a per-file basis would reduce the number of patches by only four, from 32 to 28.
In case you have the patches ready it's better to send them at once. The risk of having to change a lot of stuff is at worst the same.
participants (2)
-
Peter Krempa -
Tim Wiederhake