[libvirt] [PATCH] qemu: Dissolve qemuBuildVhostuserCommandLine in qemuBuildInterfaceCommandLine

[Erik Skultety]

On Fri, Nov 02, 2018 at 01:56:17PM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
>
> The qemuBuildVhostuserCommandLine builds command line for
> vhostuser type interfaces. It is duplicating some code of the
> function it is called from (qemuBuildInterfaceCommandLine)
> because of the way it's called. If we merge it into the caller
> not only we save a few lines but we also enable checks that we
> would have to duplicate otherwise (e.g. QoS availability).
>
> While at it, reorder some VIR_FREE() in
> qemuBuildInterfaceCommandLine so that it is easier to track which
> variables are freed and which are not.

Sounds like ^this would go into a separate trivial patch.

>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_command.c | 113 +++++++++++++++++-----------------------
>  1 file changed, 48 insertions(+), 65 deletions(-)
>
...

> @@ -8595,24 +8577,25 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
>          virSetError(saved_err);
>          virFreeError(saved_err);
>      }
> -    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
> -        if (ret < 0)
> -            VIR_FORCE_CLOSE(tapfd[i]);
> -        if (tapfdName)
> -            VIR_FREE(tapfdName[i]);
> -    }
>      for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
>          if (ret < 0)
>              VIR_FORCE_CLOSE(vhostfd[i]);
>          if (vhostfdName)
>              VIR_FREE(vhostfdName[i]);
>      }
> -    VIR_FREE(tapfd);
> +    VIR_FREE(vhostfdName);
> +    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
> +        if (ret < 0)
> +            VIR_FORCE_CLOSE(tapfd[i]);
> +        if (tapfdName)
> +            VIR_FREE(tapfdName[i]);
> +    }
> +    VIR_FREE(tapfdName);
>      VIR_FREE(vhostfd);
> -    VIR_FREE(nic);
> +    VIR_FREE(tapfd);
> +    VIR_FREE(chardev);
>      VIR_FREE(host);
> -    VIR_FREE(tapfdName);
> -    VIR_FREE(vhostfdName);
> +    VIR_FREE(nic);

I don't see how ^this hunk made it better. If anything, then the VIR_FREEs
should be probably coupled like:

VIR_FREE(tapfd);
VIR_FREE(tapfdName);
VIR_FREE(vhostfd);
VIR_FREE(vhostfdName);
<the rest of them...>

It would also need to be a separate patch. To the rest of the changes:
Reviewed-by: Erik Skultety <eskultet at redhat.com>