[libvirt] [PATCH v3 0/1] qemu_command: tidy up qemuBuildHostdevCommandLine loop
This fella got buried a few months ago. Re-sending as a v3 due to rebase changes. changes from previous version [1]: - rebased to master at 6c17606b7c [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00457.html Daniel Henrique Barboza (1): qemu_command: tidy up qemuBuildHostdevCommandLine loop src/qemu/qemu_command.c | 52 +++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 20 deletions(-) -- 2.23.0
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. All of this can be avoided by moving the hostdev->mode comparing to the start of the loop and using a switch statement with subsys->type to execute the proper code for a given hostdev type. Suggested-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_command.c | 52 +++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 836057a4ff..948ef688f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5309,23 +5309,31 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, for (i = 0; i < def->nhostdevs; i++) { virDomainHostdevDefPtr hostdev = def->hostdevs[i]; virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + virDomainHostdevSubsysSCSIPtr scsisrc; + virDomainHostdevSubsysMediatedDevPtr mdevsrc; g_autofree char *devstr = NULL; + g_autofree char *drvstr = NULL; + g_autofree char *vhostfdName = NULL; + unsigned int bootIndex; + int vhostfd = -1; - /* USB */ - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + switch (subsys->type) { + /* USB */ + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps))) return -1; virCommandAddArg(cmd, devstr); - } + + break; /* PCI */ - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { - unsigned int bootIndex = hostdev->info->bootIndex; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + bootIndex = hostdev->info->bootIndex; /* bootNet will be non-0 if boot order was set and no other * net devices were encountered @@ -5343,13 +5351,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (!devstr) return -1; virCommandAddArg(cmd, devstr); - } + + break; /* SCSI */ - if (virHostdevIsSCSIDevice(hostdev)) { - virDomainHostdevSubsysSCSIPtr scsisrc = - &hostdev->source.subsys.u.scsi; - g_autofree char *drvstr = NULL; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + scsisrc = &hostdev->source.subsys.u.scsi; if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = @@ -5372,15 +5379,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev))) return -1; virCommandAddArg(cmd, devstr); - } + + break; /* SCSI_host */ - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: if (hostdev->source.subsys.u.scsi_host.protocol == VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) { - g_autofree char *vhostfdName = NULL; - int vhostfd = -1; if (virSCSIVHostOpenVhostSCSI(&vhostfd) < 0) return -1; @@ -5399,11 +5404,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, devstr); } - } + + break; /* MDEV */ - if (virHostdevIsMdevDevice(hostdev)) { - virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + mdevsrc = &subsys->u.mdev; switch ((virMediatedDeviceModelType) mdevsrc->model) { case VIR_MDEV_MODEL_TYPE_VFIO_PCI: @@ -5422,6 +5428,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, qemuBuildHostdevMediatedDevStr(def, hostdev, qemuCaps))) return -1; virCommandAddArg(cmd, devstr); + + break; + + default: + virReportEnumRangeError(virDomainHostdevSubsysType, subsys->type); + return -1; } } -- 2.23.0
On 12/18/19 10:28 AM, 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.
All of this can be avoided by moving the hostdev->mode comparing to the start of the loop and using a switch statement with subsys->type to execute the proper code for a given hostdev type.
Suggested-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_command.c | 52 +++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 20 deletions(-)
Sorry for letting this slip review.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 836057a4ff..948ef688f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5309,23 +5309,31 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, for (i = 0; i < def->nhostdevs; i++) { virDomainHostdevDefPtr hostdev = def->hostdevs[i]; virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + virDomainHostdevSubsysSCSIPtr scsisrc; + virDomainHostdevSubsysMediatedDevPtr mdevsrc;
I think we can initialize these variables upfront, just like we are doing in virDomainAuditHostdev(), virDomainHostdevDefParseXMLSubsys(), qemuDomainGetHostdevPath() and many other places.
g_autofree char *devstr = NULL; + g_autofree char *drvstr = NULL; + g_autofree char *vhostfdName = NULL; + unsigned int bootIndex;
And also this one.
+ int vhostfd = -1;
- /* USB */ - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue;
+ switch (subsys->type) { + /* USB */ + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps))) return -1; virCommandAddArg(cmd, devstr); - } + + break;
/* PCI */ - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { - unsigned int bootIndex = hostdev->info->bootIndex; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + bootIndex = hostdev->info->bootIndex;
/* bootNet will be non-0 if boot order was set and no other * net devices were encountered @@ -5343,13 +5351,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (!devstr) return -1; virCommandAddArg(cmd, devstr); - } + + break;
/* SCSI */ - if (virHostdevIsSCSIDevice(hostdev)) { - virDomainHostdevSubsysSCSIPtr scsisrc = - &hostdev->source.subsys.u.scsi; - g_autofree char *drvstr = NULL; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + scsisrc = &hostdev->source.subsys.u.scsi;
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = @@ -5372,15 +5379,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev))) return -1; virCommandAddArg(cmd, devstr); - } + + break;
/* SCSI_host */ - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: if (hostdev->source.subsys.u.scsi_host.protocol == VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) { - g_autofree char *vhostfdName = NULL; - int vhostfd = -1;
if (virSCSIVHostOpenVhostSCSI(&vhostfd) < 0) return -1; @@ -5399,11 +5404,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, devstr); } - } + + break;
/* MDEV */ - if (virHostdevIsMdevDevice(hostdev)) { - virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + mdevsrc = &subsys->u.mdev;
switch ((virMediatedDeviceModelType) mdevsrc->model) { case VIR_MDEV_MODEL_TYPE_VFIO_PCI: @@ -5422,6 +5428,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, qemuBuildHostdevMediatedDevStr(def, hostdev, qemuCaps))) return -1; virCommandAddArg(cmd, devstr); + + break; + + default: + virReportEnumRangeError(virDomainHostdevSubsysType, subsys->type); + return -1;
If we typecast the variable used in switch() properly, then this can be turned into "case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:". I'm doing the suggested changes, ACKing and pushing. Once again, sorry. Michal
participants (2)
-
Daniel Henrique Barboza -
Michal Prívozník