Re: [libvirt PATCH v2 05/15] network: use g_auto wherever appropriate

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Tuesday in 2020, Laine Stump wrote:
> This includes standard g_autofree() as well as other objects that have
> a cleanup function defined to use via g_autoptr (virCommand,
> virJSONValue)
>
> Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> ---
> src/network/bridge_driver.c       | 206 ++++++++++--------------------
> src/network/bridge_driver_linux.c |   7 +-
> src/network/leaseshelper.c        |  16 +--
> 3 files changed, 76 insertions(+), 153 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index ab359acdb5..31bd0dd92c 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c

[...]

> @@ -1095,7 +1081,6 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
>     bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO;
>     virNetworkIPDefPtr tmpipdef, ipdef, ipv4def, ipv6def;
>     bool ipv6SLAAC;
> -    char *saddr = NULL, *eaddr = NULL;
>
>     *configstr = NULL;

[...]

> @@ -1414,6 +1396,8 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
>             int thisRange;
>             virNetworkDHCPRangeDef range = ipdef->ranges[r];
>             g_autofree char *leasetime = NULL;
> +            g_autofree char *saddr = NULL;
> +            g_autofree char *eaddr = NULL;

300 lines below the original location. Long function is long. :)

>             if (!(saddr = virSocketAddrFormat(&range.addr.start)) ||
>                 !(eaddr = virSocketAddrFormat(&range.addr.end)))

[...]

> @@ -2248,7 +2206,7 @@ static int
> networkSetIPv6Sysctls(virNetworkObjPtr obj)
> {
>     virNetworkDefPtr def = virNetworkObjGetDef(obj);
> -    char *field = NULL;
> +    g_autofree char *field = NULL;

Last time I tried manually freeing an autofree'd variable, I was told
not to do that O:-)

The clean way here seems to be refactoring the function. I can put that
somewhere into the depths of my TODO list.

>     int ret = -1;
>     bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
>
> @@ -2273,7 +2231,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>                                "on bridge %s"), field, def->bridge);
>         goto cleanup;
>     }
> -    VIR_FREE(field);
>
>     /* The rest of the ipv6 sysctl tunables should always be set the
>      * same, whether or not we're using ipv6 on this bridge.
> @@ -2282,6 +2239,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>     /* Prevent guests from hijacking the host network by sending out
>      * their own router advertisements.
>      */
> +    VIR_FREE(field);
>     field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/accept_ra",
>                             def->bridge);
>
> @@ -2290,11 +2248,11 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>                              _("cannot disable %s"), field);
>         goto cleanup;
>     }
> -    VIR_FREE(field);
>
>     /* All interfaces used as a gateway (which is what this is, by
>      * definition), must always have autoconf=0.
>      */
> +    VIR_FREE(field);
>     field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/autoconf", def->bridge);
>
>     if (virFileWriteStr(field, "0", 0) < 0) {
> @@ -2305,7 +2263,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>
>     ret = 0;
>  cleanup:
> -    VIR_FREE(field);
>     return ret;
> }

[...]

> @@ -3276,8 +3221,6 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
>                    MAX_BRIDGE_ID);
>     ret = 0;

So this function returned 0 even on failure.
Introduced by a28d3e485f01d16320af15780bc935f54782a45d

>  cleanup:
> -    if (ret < 0)
> -        VIR_FREE(newname);
>     return ret;
> }

Without the networkSetIPv6Sysctls changes:
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature