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

[Erik Skultety]

On Fri, Nov 02, 2018 at 04:01:44PM +0100, Michal Privoznik wrote:
> On 11/02/2018 03:44 PM, Erik Skultety wrote:
> > 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...>
>
> Why is that? I can see two reasonable orderings. If you have variables
> A, B, C, you either free them in the same order or in reverse C, B, A.
> Any other is just hard to follow.

Well, additionally to what you just said, if you see similarly
named  variables, it induces some kind of relation between such variables, it
may be completely misleading, but not in our case. Now, if you couple these
together in the "free block" it's imho easier for both the writer and the reader
to make sure/verify that no variable is missing from the block, so your example
becomes:

A
AA
B
BB
C

which is imho more readable than:

A
B
AA
C
BB

All in all this is a bike shed we should not spend any more minute with...

Erik