[libvirt PATCH 2/5] util: dnsmasq: refactor CapsRefresh

[Laine Stump]

On 12/13/21 1:58 PM, Ján Tomko wrote:
> Use two variables with automatic cleanup instead of reusing one.
> 
> Remove the pointless cleanup label.
> 
> Signed-off-by: Ján Tomko <jtomko at redhat.com>
> ---
>   src/util/virdnsmasq.c | 37 ++++++++++++++++---------------------
>   1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
> index 2dd9a20377..b62e353ceb 100644
> --- a/src/util/virdnsmasq.c
> +++ b/src/util/virdnsmasq.c
> @@ -666,9 +666,9 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf)
>   static int
>   dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
>   {
> -    int ret = -1;
>       struct stat sb;
> -    virCommand *cmd = NULL;
> +    g_autoptr(virCommand) vercmd = NULL;
> +    g_autoptr(virCommand) helpcmd = NULL;
>       g_autofree char *help = NULL;
>       g_autofree char *version = NULL;
>       g_autofree char *complete = NULL;
> @@ -692,31 +692,26 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
>       if (!virFileIsExecutable(caps->binaryPath)) {
>           virReportSystemError(errno, _("dnsmasq binary %s is not executable"),
>                                caps->binaryPath);
> -        goto cleanup;
> +        return -1;
>       }
>   
> -    cmd = virCommandNewArgList(caps->binaryPath, "--version", NULL);
> -    virCommandSetOutputBuffer(cmd, &version);
> -    virCommandAddEnvPassCommon(cmd);
> -    virCommandClearCaps(cmd);
> -    if (virCommandRun(cmd, NULL) < 0)
> -        goto cleanup;
> -    virCommandFree(cmd);
> +    vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL);
> +    virCommandSetOutputBuffer(vercmd, &version);
> +    virCommandAddEnvPassCommon(vercmd);
> +    virCommandClearCaps(vercmd);
> +    if (virCommandRun(vercmd, NULL) < 0)
> +        return -1;

Hmmm. Every time I run across this code, I wonder if we should keep it 
or just remove it completely - the "newest" feature we're checking for 
was added to dnsmasq in version 2.67, which was released in late 2013. 
So all these extra executions of dnsmasq to get the version# and parse 
the help output are just producing the same results for everyone.

On the other hand, it's possible some new feature could be added to 
dnsmasq in the future that we would want to check for, and that would be 
easier to add if the basic structure of the code was still here. I'm not 
sure how likely that is at this point though - dnsmasq (and libvirt's 
use of dnsmasq) is fairly mature at this point, so keeping the code is 
just creating more maintenance burden for nothing...