[libvirt] [PATCH v3 01/10] Cleanup switch statements on the hostdev subsystem type
John Ferlan
jferlan at redhat.com
Fri Nov 11 21:38:16 UTC 2016
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 at 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;
> }
>
More information about the libvir-list
mailing list