[libvirt] [PATCH v2 17/42] conf: add default: case to all switch statements
John Ferlan
jferlan at redhat.com
Tue Feb 20 15:45:34 UTC 2018
On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> Even if the compiler has validated that all enum constants have case
> statements in a switch, it is not safe to omit a default: case
> statement. When assigning a value to a variable / struct field that is
> defined with an enum type, nothing prevents an invalid value being
> assigned. So defensive code must assume existance of invalid values and
> thus all switches should have a default: case.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> src/conf/device_conf.c | 4 +-
> src/conf/domain_addr.c | 10 +-
> src/conf/domain_audit.c | 4 +
> src/conf/domain_conf.c | 468 ++++++++++++++++++++++++++++++++-------
> src/conf/domain_event.c | 1 +
> src/conf/interface_conf.c | 21 +-
> src/conf/netdev_bandwidth_conf.h | 4 +-
> src/conf/network_conf.c | 8 +-
> src/conf/network_event.c | 1 +
> src/conf/node_device_conf.c | 27 ++-
> src/conf/node_device_event.c | 1 +
> src/conf/nwfilter_conf.c | 1 +
> src/conf/nwfilter_params.c | 14 ++
> src/conf/secret_event.c | 1 +
> src/conf/storage_event.c | 1 +
> src/conf/virnodedeviceobj.c | 1 +
> src/conf/virstorageobj.c | 1 +
> 17 files changed, 468 insertions(+), 100 deletions(-)
>
Ugh... A few more DEAD_ERROR* from switch stuff - I'm just going to
disable it in my coverity check scripts. It'll be easier that way. Maybe
some day in the far future gcc8 will be used in/for Coverity and they'll
fix the problem ;-)
Given the comments about a common enum error, I'll provide some feedback
here without trying to cloud my mind with that...
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index d69f94fadf..6037d290e9 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -83,7 +83,6 @@ virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo *a,
>
> switch ((virDomainDeviceAddressType) a->type) {
> case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
> - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
LAST changes from a return true because of break ...
> /* address types below don't have any specific data */
> case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
> case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
> @@ -140,6 +139,9 @@ virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo *a,
> if (memcmp(&a->addr.dimm, &b->addr.dimm, sizeof(a->addr.dimm)))
> return false;
> break;
> + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
> + default:
> + return false;
... to a return false (and of course the default: case is added) ...
Later on in virDomainChrSourceDefIsEqual the _LAST continues to return
true when the default case is added and returns false.
> }
>
> return true;
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index df19c6be1a..98230fc267 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
[...]
> @@ -352,6 +354,11 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("PCI controller model was not set correctly"));
> return -1;
> +
> + default:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unexpected PCI controller model %d"), model);
> + return -1;
For consistency, shouldn't LAST and default elicit the same Unexpected
message?
> }
>
> bus->model = model;
> @@ -1753,6 +1760,7 @@ virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
> case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
> case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> + default:
> break;
> }
> return 0;
Could just return 0 instead of break (like done above in
virDomainPCIControllerModelToConnectType)
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 538dfc84bd..9cc6e87ccb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> @@ -2338,6 +2354,9 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
> case VIR_DOMAIN_CHR_TYPE_STDIO:
> case VIR_DOMAIN_CHR_TYPE_LAST:
> break;
> +
> + default:
> + return false;
... and here's an example where _LAST still returned true, but default
returns false
Shouldn't this follow the model of virDomainDeviceInfoAddressIsEqual
where _LAST was changed from returning TRUE (because of break) to
returning false?
IDC which way we head with this, as long as it's consistent. Someday
someone will "copy" one example or the other and someone else will note
well it's done differently elsewhere leading to the conundrum of which
approach is better.
> }
>
> return true;
[...]
> virDomainDeviceInfoPtr
> @@ -3608,6 +3636,7 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
> case VIR_DOMAIN_DEVICE_IOMMU:
> case VIR_DOMAIN_DEVICE_LAST:
> case VIR_DOMAIN_DEVICE_NONE:
> + default:
> break;
> }
> return NULL;
The return NULL could be moved inside the switch, right?
[...]
> @@ -5854,16 +5911,24 @@ virDomainDefLifecycleActionAllowed(virDomainLifecycle type,
> case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART:
> case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME:
> case VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE:
> - case VIR_DOMAIN_LIFECYCLE_ACTION_LAST:
> return true;
> case VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY:
> case VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_RESTART:
> break;
> + case VIR_DOMAIN_LIFECYCLE_ACTION_LAST:
> + default:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unexpected lifecycle action %d"), action);
> + return false;
> }
> break;
> case VIR_DOMAIN_LIFECYCLE_CRASH:
> - case VIR_DOMAIN_LIFECYCLE_LAST:
> return true;
> + case VIR_DOMAIN_LIFECYCLE_LAST:
> + default:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unexpected lifecycle type %d"), type);
> + return false;
> }
With default: in place, the following is perhaps more "readable" if it's
moved up into the one place where we break in the action switch and
could fall through. I don't think anything else can fall thru
>
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
[...]
> @@ -21433,6 +21635,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
The switch w/ VIR_DOMAIN_FEATURE_LAST is missing the default: label
>
> /* coverity[dead_error_begin] */
> case VIR_DOMAIN_HYPERV_LAST:
> + default:
> break;
> }
> }
> @@ -21457,6 +21660,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>
> /* coverity[dead_error_begin] */
> case VIR_DOMAIN_KVM_LAST:
> + default:
> break;
> }
> }
[...]
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index fd8f4e4a94..5dd945a07f 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
[...]
> @@ -2213,6 +2224,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
> case VIR_NODE_DEV_CAP_VPORTS:
> case VIR_NODE_DEV_CAP_CCW_DEV:
> case VIR_NODE_DEV_CAP_LAST:
> + default:
> /* This case is here to shutup the compiler */'
Comment can probably be removed...
> break;
> }
[...]
> diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c
> index 3a01049182..586fa539d2 100644
> --- a/src/conf/nwfilter_params.c
> +++ b/src/conf/nwfilter_params.c
[...]
>
> @@ -179,6 +182,7 @@ virNWFilterVarValueGetCardinality(const virNWFilterVarValue *val)
> return val->u.array.nValues;
> break;
> case NWFILTER_VALUE_TYPE_LAST:
> + default:
> return 0;
> }
> return 0;
Not reachable now, right?
[...]
> diff --git a/src/conf/storage_event.c b/src/conf/storage_event.c
> index f9b796878a..2f37adb875 100644
> --- a/src/conf/storage_event.c
> +++ b/src/conf/storage_event.c
> @@ -147,6 +147,7 @@ virStoragePoolEventDispatchDefaultFunc(virConnectPtr conn,
> }
>
> case VIR_STORAGE_POOL_EVENT_ID_LAST:
> + default:
> break;
> }
> VIR_WARN("Unexpected event ID %d", event->eventID);
Probably could move the VIR_WARN inside the switch now.
[...]
John
More information about the libvir-list
mailing list