[libvirt] [PATCH 1/1] qemu_command: tidy up qemuBuildHostdevCommandLine loop

[Ján Tomko]

On Wed, Oct 02, 2019 at 09:37:38AM -0300, Daniel Henrique Barboza wrote:
>The current 'for' loop with 5 consecutive 'ifs' inside
>qemuBuildHostdevCommandLine can be a bit smarter:
>
>- all 5 'ifs' fails if hostdev->mode is not equal to
>VIR_DOMAIN_HOSTDEV_MODE_SUBSYS. This check can be moved to the
>start of the loop, failing to the next element immediately
>in case it fails;
>
>- all 5 'ifs' checks for a specific subsys->type to build the proper
>command line argument (virHostdevIsSCSIDevice and virHostdevIsMdevDevice
>do that but within a helper). Problem is that the code will keep
>checking for matches even if one was already found, and there is
>no way a hostdev will fit more than one 'if' (i.e. a hostdev can't
>have 2+ different types). This means that a SUBSYS_TYPE_USB will
>create its command line argument in the first 'if', then all other
>conditionals will surely fail but will end up being checked anyway.
>This can be avoided by adding 'continue' statements inside the first
>4 conditionals.
>
>Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>---
>
>Arguably safe for freeze since it's trivial, but not a deal
>breaker if postponed. I am fine with both.
>

Whether it's applicable for freeze is a better question - postponing
refactors that do not have any functional impact does not have a benefit
for the users of that particular release and risks introducing a bug
in case we misjudged the 'no functional impact' part.

>
> src/qemu/qemu_command.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 8c979fdf65..0357481dd1 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -5471,20 +5471,23 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>         virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys;
>         VIR_AUTOFREE(char *) devstr = NULL;
>
>+        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
>+            continue;
>+

Moving the mode == SUBSYS check here does make it more readable (and
makes sense, given that we fill the subsys pointer above regardless of
the actual mode)

>         /* USB */
>-        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>-            subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
>+        if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
>
>             virCommandAddArg(cmd, "-device");
>             if (!(devstr =
>                   qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps)))
>                 return -1;
>             virCommandAddArg(cmd, devstr);
>+
>+            continue;

IMO using a switch here would be even better.

Jano

>         }
>
>         /* PCI */
>-        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>-            subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
>+        if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
>             int backend = subsys->u.pci.backend;
>
>             if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191007/3a004a8f/attachment-0001.sig>