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@xxxxxxxxxx>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
