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

[Michal Prívozník]

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 at redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413 at 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