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

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt;