[libvirt PATCH v2 00/11] 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. v1: https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html Changes since v1: * Fixed a memory leak that was introduced in patch #3 and fixed in patch #4 * Split up patches 3 and 4 into three patches Regards, Tim Tim Wiederhake (11): qemuMonitorGetAllBlockStatsInfo: Clean up line break qemuMonitorGetAllBlockStatsInfo: Remove superfluous variable initialization qemuMonitorGetAllBlockStatsInfo: Assign hash table only on success qemuMonitorGetAllBlockStatsInfo: Use automatic memory management qemuMonitorGetAllBlockStatsInfo: `virHashNew` cannot return NULL 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 | 53 ++++++++++++----------------------------- 1 file changed, 15 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 | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 19fcc5658b..d24531832b 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2155,22 +2155,24 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, bool backingChain) { int ret; + GHashTable *stats = NULL; VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain); QEMU_CHECK_MONITOR(mon); - if (!(*ret_stats = virHashNew(g_free))) + if (!(stats = virHashNew(g_free))) goto error; - ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain); + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, stats, backingChain); if (ret < 0) goto error; + *ret_stats = stats; return ret; error: - virHashFree(*ret_stats); + virHashFree(stats); *ret_stats = NULL; return -1; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_monitor.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index d24531832b..6ff7302360 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2155,26 +2155,22 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, bool backingChain) { int ret; - GHashTable *stats = NULL; + g_autoptr(GHashTable) stats = NULL; + VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain); QEMU_CHECK_MONITOR(mon); if (!(stats = virHashNew(g_free))) - goto error; + return -1; 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); - *ret_stats = 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 6ff7302360..dd0658f93c 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2155,15 +2155,12 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitor *mon, bool backingChain) { int ret; - g_autoptr(GHashTable) stats = NULL; + g_autoptr(GHashTable) stats = virHashNew(g_free); VIR_DEBUG("ret_stats=%p, backing=%d", ret_stats, backingChain); QEMU_CHECK_MONITOR(mon); - if (!(stats = virHashNew(g_free))) - return -1; - ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, stats, backingChain); if (ret < 0) -- 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 dd0658f93c..933d4a0154 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 933d4a0154..2253c96cb5 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 2253c96cb5..f2c35ab173 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 f2c35ab173..cb59fc7b7b 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2884,7 +2884,6 @@ int qemuMonitorGetChardevInfo(qemuMonitor *mon, GHashTable **retinfo) { - int ret; GHashTable *info = NULL; VIR_DEBUG("retinfo=%p", retinfo); @@ -2894,9 +2893,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 cb59fc7b7b..4489b809f4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2884,25 +2884,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
On 7/6/21 2:37 PM, Tim Wiederhake wrote:
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 cb59fc7b7b..4489b809f4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2884,25 +2884,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;
This feels redundant. In previous patches you changed the code so that the output argument is set only in case of success. I think this line should be removed.
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; }
Michal
On Tue, 2021-07-13 at 09:36 +0200, Michal Prívozník wrote:
On 7/6/21 2:37 PM, Tim Wiederhake wrote:
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 cb59fc7b7b..4489b809f4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2884,25 +2884,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;
This feels redundant. In previous patches you changed the code so that the output argument is set only in case of success. I think this line should be removed.
Previous behavior was to set *retinfo to NULL explicitly ...
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;
... here, if an error occured. From what I can tell, this is not really neccessary, feel free to merge with or without the explicit "*retinfo = NULL;". Thanks, Tim
- return -1; }
Michal
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 4489b809f4..ef24b2ca1e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2884,16 +2884,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
On 7/6/21 2:37 PM, Tim Wiederhake wrote:
"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.
v1: https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html
Changes since v1: * Fixed a memory leak that was introduced in patch #3 and fixed in patch #4 * Split up patches 3 and 4 into three patches
Regards, Tim
Tim Wiederhake (11): qemuMonitorGetAllBlockStatsInfo: Clean up line break qemuMonitorGetAllBlockStatsInfo: Remove superfluous variable initialization qemuMonitorGetAllBlockStatsInfo: Assign hash table only on success qemuMonitorGetAllBlockStatsInfo: Use automatic memory management qemuMonitorGetAllBlockStatsInfo: `virHashNew` cannot return NULL 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 | 53 ++++++++++++----------------------------- 1 file changed, 15 insertions(+), 38 deletions(-)
Looks good to me. Please let me know if you agree with my suggestion in 10/11. I can fix that before pushing. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On 7/13/21 9:36 AM, Michal Prívozník wrote:
On 7/6/21 2:37 PM, Tim Wiederhake wrote:
"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.
v1: https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html
Changes since v1: * Fixed a memory leak that was introduced in patch #3 and fixed in patch #4 * Split up patches 3 and 4 into three patches
Regards, Tim
Tim Wiederhake (11): qemuMonitorGetAllBlockStatsInfo: Clean up line break qemuMonitorGetAllBlockStatsInfo: Remove superfluous variable initialization qemuMonitorGetAllBlockStatsInfo: Assign hash table only on success qemuMonitorGetAllBlockStatsInfo: Use automatic memory management qemuMonitorGetAllBlockStatsInfo: `virHashNew` cannot return NULL 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 | 53 ++++++++++++----------------------------- 1 file changed, 15 insertions(+), 38 deletions(-)
Looks good to me. Please let me know if you agree with my suggestion in 10/11. I can fix that before pushing.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Pushed now. Michal
participants (2)
-
Michal Prívozník -
Tim Wiederhake