[libvirt PATCH 00/10] virHashNew refactorings
"virHashNew" cannot return NULL, yet we check for NULL in various places. This series is the first of several that remove these checks. Where applicable, the functions are refactored to use automatic memory management by means of g_autoptr etc. as well. Tim Wiederhake (10): qemuMonitorGetAllBlockStatsInfo: Clean up line break qemuMonitorGetAllBlockStatsInfo: Remove superfluous variable initialization qemuMonitorGetAllBlockStatsInfo: Assign hash table only on success qemuMonitorGetAllBlockStatsInfo: Use automatic memory management qemuMonitorGetBlockInfo: Remove superfluous variable "ret" qemuMonitorGetBlockInfo: Use automatic memory management qemuMonitorGetBlockInfo: `virHashNew` cannot return NULL qemuMonitorGetChardevInfo: Remove superfluous variable "ret" qemuMonitorGetChardevInfo: Use automatic memory management qemuMonitorGetChardevInfo: `virHashNew` cannot return NULL src/qemu/qemu_monitor.c | 54 ++++++++++++----------------------------- 1 file changed, 16 insertions(+), 38 deletions(-) -- 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 8f35b4240f..df1e41f68f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2162,8 +2162,7 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, if (!(*ret_stats = virHashNew(g_free))) goto error; - ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, - backingChain); + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain); if (ret < 0) goto error; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index df1e41f68f..19fcc5658b 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2154,7 +2154,7 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, GHashTable **ret_stats, bool backingChain) { - int ret = -1; + int ret; VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain); QEMU_CHECK_MONITOR(mon); -- 2.31.1
`virHashNew` cannot return NULL, the check is not needed. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 19fcc5658b..86aabc98c3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2155,23 +2155,23 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, bool backingChain) { int ret; + GHashTable *stats = virHashNew(g_free); + VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain); QEMU_CHECK_MONITOR(mon); - if (!(*ret_stats = virHashNew(g_free))) - goto error; - - ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain); + *ret_stats = NULL; + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, stats, backingChain); if (ret < 0) goto error; + *ret_stats = stats; return ret; error: - virHashFree(*ret_stats); - *ret_stats = NULL; + virHashFree(stats); return -1; } -- 2.31.1
On Mon, Jul 05, 2021 at 05:36:42PM +0200, Tim Wiederhake wrote:
`virHashNew` cannot return NULL, the check is not needed.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 19fcc5658b..86aabc98c3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2155,23 +2155,23 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, bool backingChain) { int ret; + GHashTable *stats = virHashNew(g_free); + VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain);
QEMU_CHECK_MONITOR(mon);
This macro expands with 'return -1', so this change causes a memory leak of 'stats'. This obscurity is why i don't like macros containing hidden control flow.
- if (!(*ret_stats = virHashNew(g_free))) - goto error; - - ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain); + *ret_stats = NULL; + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, stats, backingChain);
if (ret < 0) goto error;
+ *ret_stats = stats; return ret;
error: - virHashFree(*ret_stats); - *ret_stats = NULL; + virHashFree(stats); return -1; }
-- 2.31.1
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 :|
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 86aabc98c3..f08b43bbfb 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2155,7 +2155,7 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, bool backingChain) { int ret; - GHashTable *stats = virHashNew(g_free); + g_autoptr(GHashTable) stats = virHashNew(g_free); VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain); @@ -2165,14 +2165,10 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, stats, backingChain); if (ret < 0) - goto error; + return -1; - *ret_stats = stats; + *ret_stats = g_steal_pointer(&stats); return ret; - - error: - virHashFree(stats); - return -1; } -- 2.31.1
On Mon, Jul 05, 2021 at 05:36:43PM +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 86aabc98c3..f08b43bbfb 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2155,7 +2155,7 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, bool backingChain) { int ret; - GHashTable *stats = virHashNew(g_free); + g_autoptr(GHashTable) stats = virHashNew(g_free);
VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain);
@@ -2165,14 +2165,10 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, stats, backingChain);
if (ret < 0) - goto error; + return -1;
- *ret_stats = stats; + *ret_stats = g_steal_pointer(&stats); return ret; - - error: - virHashFree(stats); - return -1; }
This fixes the bug introduced by the previous patch. The changes need to be reversed to ensure bisect works correctly. 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 :|
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 f08b43bbfb..de0f6ccf7d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2103,7 +2103,6 @@ qemuDomainDiskInfoFree(void *value) GHashTable * qemuMonitorGetBlockInfo(qemuMonitor *mon) { - int ret; GHashTable *table; QEMU_CHECK_MONITOR_NULL(mon); @@ -2111,9 +2110,7 @@ qemuMonitorGetBlockInfo(qemuMonitor *mon) if (!(table = virHashNew(qemuDomainDiskInfoFree))) return NULL; - ret = qemuMonitorJSONGetBlockInfo(mon, table); - - if (ret < 0) { + if (qemuMonitorJSONGetBlockInfo(mon, table) < 0) { virHashFree(table); return NULL; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index de0f6ccf7d..9f0b20db09 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2103,7 +2103,7 @@ qemuDomainDiskInfoFree(void *value) GHashTable * qemuMonitorGetBlockInfo(qemuMonitor *mon) { - GHashTable *table; + g_autoptr(GHashTable) table = NULL; QEMU_CHECK_MONITOR_NULL(mon); @@ -2111,11 +2111,10 @@ qemuMonitorGetBlockInfo(qemuMonitor *mon) return NULL; if (qemuMonitorJSONGetBlockInfo(mon, table) < 0) { - virHashFree(table); return NULL; } - return table; + return g_steal_pointer(&table); } -- 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 9f0b20db09..dba2fb1982 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2103,13 +2103,10 @@ qemuDomainDiskInfoFree(void *value) GHashTable * qemuMonitorGetBlockInfo(qemuMonitor *mon) { - g_autoptr(GHashTable) table = NULL; + g_autoptr(GHashTable) table = virHashNew(qemuDomainDiskInfoFree); QEMU_CHECK_MONITOR_NULL(mon); - if (!(table = virHashNew(qemuDomainDiskInfoFree))) - return NULL; - if (qemuMonitorJSONGetBlockInfo(mon, table) < 0) { return NULL; } -- 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 dba2fb1982..f03d6106f9 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2885,7 +2885,6 @@ int qemuMonitorGetChardevInfo(qemuMonitor *mon, GHashTable **retinfo) { - int ret; GHashTable *info = NULL; VIR_DEBUG("retinfo=%p", retinfo); @@ -2895,9 +2894,7 @@ qemuMonitorGetChardevInfo(qemuMonitor *mon, if (!(info = virHashNew(qemuMonitorChardevInfoFree))) goto error; - ret = qemuMonitorJSONGetChardevInfo(mon, info); - - if (ret < 0) + if (qemuMonitorJSONGetChardevInfo(mon, info) < 0) goto error; *retinfo = info; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f03d6106f9..2c43cc0788 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2885,25 +2885,21 @@ int qemuMonitorGetChardevInfo(qemuMonitor *mon, GHashTable **retinfo) { - GHashTable *info = NULL; + g_autoptr(GHashTable) info = NULL; VIR_DEBUG("retinfo=%p", retinfo); - QEMU_CHECK_MONITOR_GOTO(mon, error); + QEMU_CHECK_MONITOR(mon); + *retinfo = NULL; if (!(info = virHashNew(qemuMonitorChardevInfoFree))) - goto error; + return -1; if (qemuMonitorJSONGetChardevInfo(mon, info) < 0) - goto error; + return -1; - *retinfo = info; + *retinfo = g_steal_pointer(&info); return 0; - - error: - virHashFree(info); - *retinfo = NULL; - return -1; } -- 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 2c43cc0788..a63056f0a0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2885,16 +2885,13 @@ int qemuMonitorGetChardevInfo(qemuMonitor *mon, GHashTable **retinfo) { - g_autoptr(GHashTable) info = NULL; + g_autoptr(GHashTable) info = virHashNew(qemuMonitorChardevInfoFree); VIR_DEBUG("retinfo=%p", retinfo); QEMU_CHECK_MONITOR(mon); *retinfo = NULL; - if (!(info = virHashNew(qemuMonitorChardevInfoFree))) - return -1; - if (qemuMonitorJSONGetChardevInfo(mon, info) < 0) return -1; -- 2.31.1
participants (2)
-
Daniel P. Berrangé -
Tim Wiederhake