Re: [PATCH v3 01/10] Cleanup switch statements on the hostdev subsystem type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 11/08/2016 01:26 PM, Eric Farman wrote:
> As was suggested in an earlier review comment[1], we can
> catch some additional code points by cleaning up how we use the
> hostdev subsystem type in some switch statements.
> 
> [1] End of https://www.redhat.com/archives/libvir-list/2016-September/msg00399.html
> 
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxxxxxxx>
> ---
>  src/conf/domain_conf.c           | 11 +++++++++--
>  src/qemu/qemu_cgroup.c           | 11 +++++++----
>  src/security/security_apparmor.c |  4 ++--
>  src/security/security_selinux.c  |  8 ++++----
>  4 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a233c0c..043f0e2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12992,7 +12992,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt,
>      }
>  
>      if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> -        switch (def->source.subsys.type) {
> +        switch ((virDomainHostdevSubsysType) def->source.subsys.type) {
>          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>              if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>                  def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> @@ -13014,6 +13014,11 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt,
>              if (virXPathBoolean("boolean(./shareable)", ctxt))
>                  def->shareable = true;
>              break;
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> +            break;
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> +            goto error;

Since virDomainHostdevDefParseXMLSubsys will validate that 'type' is
valid w/ a call to virDomainHostdevSubsysTypeFromString, that means for
this code there's no way the subsys.type could be invalid. So no need to
jump to error.  FWIW: Even if you did jump to error, there should be an
error message.

In fact both USB and LAST "fall through" for now.

Later though when SCSI_HOST is added, that's when you can do some more
address validation since it'll be a new type. As opposed to the lack of
validation for USB since USB existed before we added having <address>
validation (which is in the post parse callback IIRC).

I will adjust this before pushing (shortly)...

ACK -

John

> +            break;
>          }
>      }
>  
> @@ -13880,7 +13885,7 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>      if (a->source.subsys.type != b->source.subsys.type)
>          return 0;
>  
> -    switch (a->source.subsys.type) {
> +    switch ((virDomainHostdevSubsysType) a->source.subsys.type) {
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>          return virDomainHostdevMatchSubsysPCI(a, b);
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> @@ -13894,6 +13899,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>              return virDomainHostdevMatchSubsysSCSIiSCSI(a, b);
>          else
>              return virDomainHostdevMatchSubsysSCSIHost(a, b);
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> +        return 0;
>      }
>      return 0;
>  }
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 4489c64..1443f7e 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -301,7 +301,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>  
>      if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
>  
> -        switch (dev->source.subsys.type) {
> +        switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
>          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>              if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
>                  int rv;
> @@ -376,7 +376,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>              break;
>          }
>  
> -        default:
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>              break;
>          }
>      }
> @@ -410,7 +410,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
>  
>      if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
>  
> -        switch (dev->source.subsys.type) {
> +        switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
>          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>              if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
>                  int rv;
> @@ -437,7 +437,10 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
>          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
>              /* nothing to tear down for USB */
>              break;
> -        default:
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> +            /* nothing to tear down for SCSI */
> +            break;
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>              break;
>          }
>      }
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index af2b639..beddf6d 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -856,7 +856,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
>      ptr->mgr = mgr;
>      ptr->def = def;
>  
> -    switch (dev->source.subsys.type) {
> +    switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
>          virUSBDevicePtr usb =
>              virUSBDeviceNew(usbsrc->bus, usbsrc->device, vroot);
> @@ -909,7 +909,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    default:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
>          break;
>      }
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 5dad22c..89c93dc 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1436,7 +1436,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
>          scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
>          return 0;
>  
> -    switch (dev->source.subsys.type) {
> +    switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
>          virUSBDevicePtr usb;
>  
> @@ -1498,7 +1498,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    default:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
>          break;
>      }
> @@ -1640,7 +1640,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
>          scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
>          return 0;
>  
> -    switch (dev->source.subsys.type) {
> +    switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
>          virUSBDevicePtr usb;
>  
> @@ -1700,7 +1700,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    default:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
>          break;
>      }
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]