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@linux.vnet.ibm.com> --- 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; }