11:18 a.m.
On 7/15/20 11:10 AM, Ján Tomko wrote:
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(a)redhat.com>
> ---
> 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. :)
At least there were no unrelated changes in be... oh, wait. Nevermind.
A long time ago (1988) in a galaxy far far away (Lake City, Minnesota) I
worked with a guy who told me that any function that wouldn't fit on a
single screen was too long and needed to be broken up (this was in the
80x25 ASCII terminal days). He would probably rip out his moustache and
scream if he saw some of the functions in libvirt.
>
> 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:-)
Yeah, there's a few places where we re-use a pointer for "temporary"
strings. In a different patch I sent a few weeks ago, I fixed it by just
declaring multiple separate autofree variables, one for each usage, and
I think that is the least future-bug-prone method of dealing with it.
(I hadn't seen anyone scolding about not manually freeing and autofree'd
variable, but since doing so made me uneasy anyway, I'm happy to jump on
the bandwagon :-)
The clean way here seems to be refactoring the function. I can put that
somewhere into the depths of my TODO list.
If you really want to, you can. Otherwise I can send a patch for that to
be pushed along with this series, just so that I won't have the icky
feeling of leaving a job not quite done.
> 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(a)redhat.com>
Jano