Re: [PATCH 3/4] Use g_autoptr instead of virNetDevBandwidthFree where possible

[Ján Tomko]

On a Tuesday in 2021, Kristina Hanicova wrote:

In files: netdev_bandwidth_conf: in virNetDevBandwidthParse(),
bridge_driver: in networkPortSetParameters(), qemu_driver: in
qemuDomainSetInterfaceParameters(), test_driver: in
testDomainSetInterfaceParameters(), virnetdevbandwidthtest: in
testVirNetDevBandwidthSet()

For lists in commit messages, bullet lists are more readable:
* virNetDevBandwidthParse
* networkPortSetParameters

Listing the changed files is not needed with git, usually people
who see the commit message can see the diff list below (or use --stat):

Signed-off-by: Kristina Hanicova <khani...@redhat.com>
---
src/conf/netdev_bandwidth_conf.c |  9 +++------
src/network/bridge_driver.c      |  6 ++----
src/qemu/qemu_driver.c           | 11 ++++-------
src/test/test_driver.c           |  3 +--
src/util/virnetdevbandwidth.h    |  2 ++
tests/virnetdevbandwidthtest.c   |  3 +--
6 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c
index 1ff3785677..81590efe6d 100644
--- a/src/conf/netdev_bandwidth_conf.c
+++ b/src/conf/netdev_bandwidth_conf.c
@@ -111,7 +111,7 @@ virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
                        bool allowFloor)
{
    int ret = -1;
-    virNetDevBandwidthPtr def = NULL;
+    g_autoptr(virNetDevBandwidth) def = NULL;
    xmlNodePtr cur;
    xmlNodePtr in = NULL, out = NULL;
    g_autofree char *class_id_prop = NULL;
@@ -197,15 +197,12 @@ virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
        }
    }

-    if (!def->in && !def->out)
-        VIR_FREE(def);
+    if (def->in || def->out)
+        *bandwidth = g_steal_pointer(&def);

-    *bandwidth = def;
-    def = NULL;
    ret = 0;

This hunk combines:
* removal of VIR_FREE
* inversion of the condition
* the g_steal_pointer change (which is also included in your other
   patch)

For mass conversions across multiple functions or files, doing one type
of change at a time is easier to read (and review).

Alternatively, if a particular function needs more love, consider
sending it as a separate "Refactor virNetDevBandwidthParse" commit.

 cleanup:
-    virNetDevBandwidthFree(def);
    return ret;
}

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 519a473995..b29c37ef4c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5397,7 +5397,7 @@ networkPortSetParameters(virNetworkPortPtr port,
    virNetworkObjPtr obj;
    virNetworkDefPtr def;
    virNetworkPortDefPtr portdef;
-    virNetDevBandwidthPtr bandwidth = NULL;
+    g_autoptr(virNetDevBandwidth) bandwidth = NULL;
    g_autofree char *dir = NULL;
    int ret = -1;
    size_t i;
@@ -5466,15 +5466,13 @@ networkPortSetParameters(virNetworkPortPtr port,
        goto cleanup;

    virNetDevBandwidthFree(portdef->bandwidth);

-    portdef->bandwidth = bandwidth;
-    bandwidth = NULL;
+    portdef->bandwidth = g_steal_pointer(&bandwidth);

These g_steal_pointer conversions are also not needed to use g_auto and
can be separated.

Jano

    if (virNetworkPortDefSaveStatus(portdef, dir) < 0)
        goto cleanup;

    ret = 0;
 cleanup:
-    virNetDevBandwidthFree(bandwidth);
    virNetworkObjEndAPI(&obj);
    return ret;
}

signature.asc
Description: PGP signature