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

[Ján Tomko <jtomko@xxxxxxxxxx>]

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 <khanicov@xxxxxxxxxx>
> ---
> 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;
> }

Attachment: signature.asc
Description: PGP signature