[libvirt] [PATCH 1/1] qemu_command: tidy up qemuBuildHostdevCommandLine loop
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@gmail.com> --- Arguably safe for freeze since it's trivial, but not a deal breaker if postponed. I am fine with both. 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; + /* 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; } /* 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) { @@ -5514,6 +5517,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (!devstr) return -1; virCommandAddArg(cmd, devstr); + + continue; } /* SCSI */ @@ -5543,11 +5548,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev))) return -1; virCommandAddArg(cmd, devstr); + + continue; } /* SCSI_host */ - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) { + if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) { if (hostdev->source.subsys.u.scsi_host.protocol == VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) { VIR_AUTOFREE(char *) vhostfdName = NULL; @@ -5573,6 +5579,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, devstr); } + + continue; } /* MDEV */ -- 2.21.0
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@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) {
participants (2)
-
Daniel Henrique Barboza -
Ján Tomko