[libvirt] [PATCH] bridge_driver: cleanup improvements in dhcpStartDhcpDaemon()

[Matthias Bolte]

2010/12/21 Paweł Krześniak <pawel.krzesniak at gmail.com>:
> Run VIR_FREE only for non-NULL pointers.
> ---
>  src/network/bridge_driver.c |   17 +++++++++--------
>  1 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index b0834ae..f2857b4 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -534,34 +534,34 @@ dhcpStartDhcpDaemon(virNetworkObjPtr network)
>     if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET)) {
>         networkReportError(VIR_ERR_INTERNAL_ERROR,
>                            "%s", _("cannot start dhcp daemon without
> IPv4 address for server"));
> -        goto cleanup;
> +        goto cleanup2;
>     }

>     ret = 0;
> -cleanup:
> +cleanup1:
>     VIR_FREE(pidfile);
>     virCommandFree(cmd);
> +cleanup2:
>     return ret;
>  }

NACK as written, because you're complicating this function
unnecessarily. It's totally fine to call VIR_FREE on a possible NULL
pointer without checking, this is the common pattern in libvirt. Your
additional label resembles the logic of

if (pointer != NULL) {
    VIR_FREE(pointer);
}

This pattern in explicitly banned in libvirt by the make syntax-check
rules and should to be simplified to

VIR_FREE(pointer);

as VIR_FREE (just like the normal free function) is safe to be called
on a NULL pointer.

If you really insists in avoiding the VIR_FREE call in the error path
when the pointer is NULL, then you could just replace your goto
cleanup2 by return -1, otherwise I suggest to leave this function as
is, because there is nothing to fix here.

Matthias