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

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(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...> 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.