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