4:55 a.m.
"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
4:55 a.m.
New subject: [libvirt PATCH 01/10] qemuStateInitialize: `virHashNew` cannot return NULL
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 02/10] qemuMonitorGetPRManagerInfo: `virHashNew` cannot return NULL
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 03/10] qemuMonitorGetPRManagerInfo: Use automatic memory management
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 04/10] qemuMonitorGetPRManagerInfo: Remove superfluous `goto`s
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 05/10] qemuMonitorJSONGetAllBlockJobInfo: `virHashNew` cannot return NULL
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 06/10] qemuMonitorJSONGetAllBlockJobInfo: Use automatic memory management
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 07/10] qemuMonitorJSONGetAllBlockJobInfo: Remove superfluous `goto`s
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 08/10] testQemuGetLatestCaps: `virHashNew` cannot return NULL
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 09/10] testQemuGetLatestCaps: Use automatic memory management
Signed-off-by: Tim Wiederhake <twiederh(a)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
4:55 a.m.
New subject: [libvirt PATCH 10/10] testQemuGetLatestCaps: Remove superfluous `goto`s
Signed-off-by: Tim Wiederhake <twiederh(a)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
7:08 a.m.
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(a)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.
7:36 a.m.
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(a)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
5:54 a.m.
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(a)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.
1222
days inactive
1224
days old
13 comments
2 participants
participants (2)
-
Peter Krempa -
Tim Wiederhake