[libvirt] [PATCH v2 00/14] Enforce presence of all switch enum cases
This is the same as the previous series, but with standardized error reporting macro to get consistent error messages. This is just the first part of the previous series. The second part is much more work to convert to this new error reporting macro, so i'll drip-feed the rest for review over next few days... Daniel P. Berrangé (14): util: add a virReportEnumRangeError for bad value reporting util: handle missing switch enum cases conf: handle missing switch enum cases esx: handle missing switch enum cases hyperv: handle missing switch enum cases libxl: handle missing switch enum cases lxc: handle missing switch enum cases nwfilter: handle missing switch enum cases qemu: handle missing switch enum cases rpc: handle missing switch enum cases security: handle missing switch enum cases xen: handle missing switch enum cases tools: handle missing switch enum cases m4: enforce that all enum cases are listed in switch statements m4/virt-compile-warnings.m4 | 7 +++-- src/conf/domain_audit.c | 1 + src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++---- src/conf/nwfilter_conf.c | 31 ++++++++++++++++++++- src/conf/nwfilter_conf.h | 4 +-- src/esx/esx_driver.c | 1 + src/esx/esx_vi.c | 11 +++++--- src/esx/esx_vi_types.c | 9 +++--- src/hyperv/hyperv_driver.c | 18 ++++++++++-- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_container.c | 8 +++--- src/lxc/lxc_controller.c | 8 +++++- src/lxc/lxc_driver.c | 38 ++++++++++++++++++++++--- src/nwfilter/nwfilter_ebiptables_driver.c | 17 ++++++++---- src/nwfilter/nwfilter_learnipaddr.c | 6 +++- src/qemu/qemu_command.c | 26 +++++++++++------ src/qemu/qemu_domain.c | 21 ++++++++++++++ src/qemu/qemu_driver.c | 25 +++++++++++------ src/qemu/qemu_hotplug.c | 33 ++++++++++++++++++---- src/qemu/qemu_migration.c | 11 +++++++- src/qemu/qemu_process.c | 2 ++ src/rpc/virnetclient.c | 2 ++ src/rpc/virnetclientprogram.c | 1 + src/rpc/virnetserverprogram.c | 4 +++ src/security/security_driver.c | 1 + src/util/virconf.c | 11 +++++++- src/util/virerror.h | 9 +++++- src/util/virfirewall.c | 6 ++-- src/util/virlog.c | 9 +++++- src/util/virnetdevvportprofile.c | 10 ++++++- src/vmx/vmx.c | 26 +++++++++++++++-- src/xenconfig/xen_common.c | 18 ++++++++++-- src/xenconfig/xen_xl.c | 7 ++++- tools/virt-host-validate-qemu.c | 3 +- 34 files changed, 359 insertions(+), 73 deletions(-) -- 2.14.3
To ensure we have standardized error messages when reporting problems with enum values being out of a range, add virReportEnumRangeError(). virReportEnumRangeError(virDomainState, 34); results in a message "internal error: Unexpected enum value 34 for virDomainState" Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virerror.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/util/virerror.h b/src/util/virerror.h index cf434f45fc..1d02451604 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -164,7 +164,14 @@ void virReportSystemErrorFull(int domcode, # define virReportRestrictedError(...) \ virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_OPERATION_DENIED, \ __FILE__, __FUNCTION__, __LINE__, __VA_ARGS__) - +/* The sizeof(typname) comparison here is a hack to catch typos + * in the name of the enum by triggering a compile error. It should + * get optimized away since sizeof() is known at compile time */ +# define virReportEnumRangeError(typname, value) \ + virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, \ + __FILE__, __FUNCTION__, __LINE__, \ + "Unexpected enum value %d for %s", \ + value, sizeof(typname) != 0 ? #typname : #typname); void virReportOOMErrorFull(int domcode, const char *filename, -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
To ensure we have standardized error messages when reporting problems with enum values being out of a range, add virReportEnumRangeError().
virReportEnumRangeError(virDomainState, 34);
results in a message
"internal error: Unexpected enum value 34 for virDomainState"
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virerror.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/util/virerror.h b/src/util/virerror.h index cf434f45fc..1d02451604 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -164,7 +164,14 @@ void virReportSystemErrorFull(int domcode, # define virReportRestrictedError(...) \ virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_OPERATION_DENIED, \ __FILE__, __FUNCTION__, __LINE__, __VA_ARGS__) - +/* The sizeof(typname) comparison here is a hack to catch typos + * in the name of the enum by triggering a compile error. It should + * get optimized away since sizeof() is known at compile time */ +# define virReportEnumRangeError(typname, value) \ + virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, \ + __FILE__, __FUNCTION__, __LINE__, \ + "Unexpected enum value %d for %s", \
Need to resolve syntax-check warning about right-aligned "\"...
+ value, sizeof(typname) != 0 ? #typname : #typname);
I saw some internal IRC over the sizeof check... Assuming Eric and you come up with something reasonable... Reviewed-by: John Ferlan <jferlan@redhat.com> John Even though I'm not 100% on board with the whole error message for LAST and default especially when they're a cannot get there...
void virReportOOMErrorFull(int domcode, const char *filename,
Ensure all enum cases are listed in switch statements. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virconf.c | 11 ++++++++++- src/util/virfirewall.c | 6 ++++-- src/util/virlog.c | 9 ++++++++- src/util/virnetdevvportprofile.c | 10 +++++++++- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/util/virconf.c b/src/util/virconf.c index a82a509ca3..e0a3fd12c0 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -296,7 +296,9 @@ virConfSaveValue(virBufferPtr buf, virConfValuePtr val) virBufferAddLit(buf, " ]"); break; } + case VIR_CONF_LAST: default: + virReportEnumRangeError(virConfType, val->type); return -1; } return 0; @@ -986,13 +988,20 @@ int virConfGetValueStringList(virConfPtr conf, } ATTRIBUTE_FALLTHROUGH; - default: + case VIR_CONF_LLONG: + case VIR_CONF_ULLONG: + case VIR_CONF_NONE: virReportError(VIR_ERR_INTERNAL_ERROR, compatString ? _("%s: expected a string or string list for '%s' parameter") : _("%s: expected a string list for '%s' parameter"), conf->filename, setting); return -1; + + case VIR_CONF_LAST: + default: + virReportEnumRangeError(virConfType, cval->type); + return -1; } return 1; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index ff1bb83073..10c370a2ae 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -827,9 +827,11 @@ virFirewallApplyRule(virFirewallPtr firewall, if (virFirewallApplyRuleFirewallD(rule, ignoreErrors, &output) < 0) return -1; break; + + case VIR_FIREWALL_BACKEND_AUTOMATIC: + case VIR_FIREWALL_BACKEND_LAST: default: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected firewall engine backend")); + virReportEnumRangeError(virFirewallBackend, currentBackend); return -1; } diff --git a/src/util/virlog.c b/src/util/virlog.c index 4f66cc5e5c..6c6d7e8ded 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1206,10 +1206,17 @@ virLogGetOutputs(void) virLogDestinationTypeToString(dest), virLogOutputs[i]->name); break; - default: + case VIR_LOG_TO_STDERR: + case VIR_LOG_TO_JOURNALD: virBufferAsprintf(&outputbuf, "%d:%s", virLogOutputs[i]->priority, virLogDestinationTypeToString(dest)); + break; + case VIR_LOG_TO_OUTPUT_LAST: + default: + virReportEnumRangeError(virLogDestination, dest); + virLogUnlock(); + return NULL; } } virLogUnlock(); diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index db495a7549..dc774407df 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -1071,6 +1071,9 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname, case VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE: op = PORT_REQUEST_PREASSOCIATE; break; + case VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR: + op = PORT_REQUEST_PREASSOCIATE_RR; + break; case VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE: op = PORT_REQUEST_ASSOCIATE; break; @@ -1191,10 +1194,15 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname, false); break; - default: + case VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE: virReportError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); rc = -1; + break; + default: + virReportEnumRangeError(virNetDevVPortProfile, virtPortOp); + rc = -1; + break; } cleanup: -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements.
Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virconf.c | 11 ++++++++++- src/util/virfirewall.c | 6 ++++-- src/util/virlog.c | 9 ++++++++- src/util/virnetdevvportprofile.c | 10 +++++++++- 4 files changed, 31 insertions(+), 5 deletions(-)
[...]
diff --git a/src/util/virlog.c b/src/util/virlog.c index 4f66cc5e5c..6c6d7e8ded 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1206,10 +1206,17 @@ virLogGetOutputs(void) virLogDestinationTypeToString(dest), virLogOutputs[i]->name); break; - default: + case VIR_LOG_TO_STDERR: + case VIR_LOG_TO_JOURNALD: virBufferAsprintf(&outputbuf, "%d:%s", virLogOutputs[i]->priority, virLogDestinationTypeToString(dest)); + break; + case VIR_LOG_TO_OUTPUT_LAST: + default: + virReportEnumRangeError(virLogDestination, dest);
Didn't see this before, but because this is a for loop, you'll need to add a virBufferFreeAndReset(&outputbuf); before returning. Existing R-b still applies John
+ virLogUnlock(); + return NULL; } } virLogUnlock();
On Tue, Feb 20, 2018 at 01:44:31PM -0500, John Ferlan wrote:
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements.
Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virconf.c | 11 ++++++++++- src/util/virfirewall.c | 6 ++++-- src/util/virlog.c | 9 ++++++++- src/util/virnetdevvportprofile.c | 10 +++++++++- 4 files changed, 31 insertions(+), 5 deletions(-)
[...]
diff --git a/src/util/virlog.c b/src/util/virlog.c index 4f66cc5e5c..6c6d7e8ded 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1206,10 +1206,17 @@ virLogGetOutputs(void) virLogDestinationTypeToString(dest), virLogOutputs[i]->name); break; - default: + case VIR_LOG_TO_STDERR: + case VIR_LOG_TO_JOURNALD: virBufferAsprintf(&outputbuf, "%d:%s", virLogOutputs[i]->priority, virLogDestinationTypeToString(dest)); + break; + case VIR_LOG_TO_OUTPUT_LAST: + default: + virReportEnumRangeError(virLogDestination, dest);
Didn't see this before, but because this is a for loop, you'll need to add a virBufferFreeAndReset(&outputbuf); before returning.
Ok Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Ensure all enum cases are listed in switch statements. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_audit.c | 1 + src/conf/domain_conf.c | 46 ++++++++++++++++++++++++++++++++++++++++------ src/conf/nwfilter_conf.c | 31 ++++++++++++++++++++++++++++++- 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 723c737363..82868bca76 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -586,6 +586,7 @@ virDomainAuditTPM(virDomainObjPtr vm, virDomainTPMDefPtr tpm, "virt=%s resrc=dev reason=%s %s uuid=%s %s", virt, reason, vmname, uuidstr, device); break; + case VIR_DOMAIN_TPM_TYPE_LAST: default: break; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 68a8d39016..f2ddde7a36 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11567,8 +11567,20 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->filterparams = filterparams; filterparams = NULL; break; - default: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: break; + case VIR_DOMAIN_NET_TYPE_LAST: + default: + virReportEnumRangeError(virDomainNetType, def->type); + goto error; } } @@ -14703,6 +14715,12 @@ virDomainVideoDefaultRAM(const virDomainDef *def, /* QEMU use 64M as the minimal video memory for qxl device */ return 64 * 1024; + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: + case VIR_DOMAIN_VIDEO_TYPE_VBOX: + case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: + case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: + case VIR_DOMAIN_VIDEO_TYPE_GOP: + case VIR_DOMAIN_VIDEO_TYPE_LAST: default: return 0; } @@ -14737,6 +14755,16 @@ virDomainVideoDefaultType(const virDomainDef *def) return VIR_DOMAIN_VIDEO_TYPE_PARALLELS; case VIR_DOMAIN_VIRT_BHYVE: return VIR_DOMAIN_VIDEO_TYPE_GOP; + case VIR_DOMAIN_VIRT_QEMU: + case VIR_DOMAIN_VIRT_KQEMU: + case VIR_DOMAIN_VIRT_KVM: + case VIR_DOMAIN_VIRT_LXC: + case VIR_DOMAIN_VIRT_UML: + case VIR_DOMAIN_VIRT_OPENVZ: + case VIR_DOMAIN_VIRT_HYPERV: + case VIR_DOMAIN_VIRT_PHYP: + case VIR_DOMAIN_VIRT_NONE: + case VIR_DOMAIN_VIRT_LAST: default: return VIR_DOMAIN_VIDEO_TYPE_DEFAULT; } @@ -27878,7 +27906,7 @@ virDomainObjGetState(virDomainObjPtr dom, int *reason) void virDomainObjSetState(virDomainObjPtr dom, virDomainState state, int reason) { - int last = -1; + int last; switch (state) { case VIR_DOMAIN_NOSTATE: @@ -27905,11 +27933,8 @@ virDomainObjSetState(virDomainObjPtr dom, virDomainState state, int reason) case VIR_DOMAIN_PMSUSPENDED: last = VIR_DOMAIN_PMSUSPENDED_LAST; break; + case VIR_DOMAIN_LAST: default: - last = -1; - } - - if (last < 0) { VIR_ERROR(_("invalid domain state: %d"), state); return; } @@ -28073,6 +28098,15 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) default: return NULL; } + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: default: return NULL; } diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 5d6423e060..fd42d58c2c 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2153,8 +2153,34 @@ virNWFilterRuleValidate(virNWFilterRuleDefPtr rule) } } break; - default: + case VIR_NWFILTER_RULE_PROTOCOL_NONE: + case VIR_NWFILTER_RULE_PROTOCOL_MAC: + case VIR_NWFILTER_RULE_PROTOCOL_VLAN: + case VIR_NWFILTER_RULE_PROTOCOL_STP: + case VIR_NWFILTER_RULE_PROTOCOL_ARP: + case VIR_NWFILTER_RULE_PROTOCOL_RARP: + case VIR_NWFILTER_RULE_PROTOCOL_TCP: + case VIR_NWFILTER_RULE_PROTOCOL_ICMP: + case VIR_NWFILTER_RULE_PROTOCOL_IGMP: + case VIR_NWFILTER_RULE_PROTOCOL_UDP: + case VIR_NWFILTER_RULE_PROTOCOL_UDPLITE: + case VIR_NWFILTER_RULE_PROTOCOL_ESP: + case VIR_NWFILTER_RULE_PROTOCOL_AH: + case VIR_NWFILTER_RULE_PROTOCOL_SCTP: + case VIR_NWFILTER_RULE_PROTOCOL_ALL: + case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: + case VIR_NWFILTER_RULE_PROTOCOL_ICMPV6: + case VIR_NWFILTER_RULE_PROTOCOL_UDPoIPV6: + case VIR_NWFILTER_RULE_PROTOCOL_UDPLITEoIPV6: + case VIR_NWFILTER_RULE_PROTOCOL_ESPoIPV6: + case VIR_NWFILTER_RULE_PROTOCOL_AHoIPV6: + case VIR_NWFILTER_RULE_PROTOCOL_SCTPoIPV6: + case VIR_NWFILTER_RULE_PROTOCOL_ALLoIPV6: break; + case VIR_NWFILTER_RULE_PROTOCOL_LAST: + default: + virReportEnumRangeError(virNWFilterRuleProtocolType, rule->prtclType); + return -1; } return ret; @@ -3064,7 +3090,10 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf, virBufferAddLit(buf, "false"); break; + case DATATYPE_IPSETNAME: + case DATATYPE_IPSETFLAGS: case DATATYPE_STRING: + case DATATYPE_LAST: default: virBufferAsprintf(buf, "UNSUPPORTED DATATYPE 0x%02x\n", -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_audit.c | 1 + src/conf/domain_conf.c | 46 ++++++++++++++++++++++++++++++++++++++++------ src/conf/nwfilter_conf.c | 31 ++++++++++++++++++++++++++++++- 3 files changed, 71 insertions(+), 7 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Ensure all enum cases are listed in switch statements, or explicitly cast away enum type where we don't want to list all cases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/esx/esx_driver.c | 1 + src/esx/esx_vi.c | 11 +++++++---- src/esx/esx_vi_types.c | 9 +++++---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index f575362059..c5f04efa81 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3603,6 +3603,7 @@ esxDomainGetSchedulerParametersFlags(virDomainPtr domain, params[i].value.i = -3; break; + case esxVI_SharesLevel_Undefined: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Shares level has unknown value %d"), diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index edf52ff828..103f726069 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -483,7 +483,7 @@ esxVI_SharedCURL_Lock(CURL *handle ATTRIBUTE_UNUSED, curl_lock_data data, size_t i; esxVI_SharedCURL *shared = userptr; - switch (data) { + switch ((int)data) { case CURL_LOCK_DATA_SHARE: i = 0; break; @@ -511,7 +511,7 @@ esxVI_SharedCURL_Unlock(CURL *handle ATTRIBUTE_UNUSED, curl_lock_data data, size_t i; esxVI_SharedCURL *shared = userptr; - switch (data) { + switch ((int)data) { case CURL_LOCK_DATA_SHARE: i = 0; break; @@ -1563,6 +1563,7 @@ esxVI_Context_Execute(esxVI_Context *ctx, const char *methodName, break; + case esxVI_Occurrence_Undefined: default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument (occurrence)")); @@ -2280,9 +2281,11 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx, type, root->type); break; + case esxVI_Occurrence_None: + case esxVI_Occurrence_Undefined: default: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Invalid occurrence value")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid occurrence value %d"), occurrence); break; } diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c index be35af861c..74183f8307 100644 --- a/src/esx/esx_vi_types.c +++ b/src/esx/esx_vi_types.c @@ -544,7 +544,7 @@ VIR_LOG_INIT("esx.esx_vi_types"); #define ESX_VI__TEMPLATE__DISPATCH(_actual_type, _actual_type_name, __type, \ _dispatch, _error_return) \ - switch (_actual_type) { \ + switch ((int)_actual_type) { \ _dispatch \ \ case esxVI_Type_##__type: \ @@ -690,7 +690,7 @@ VIR_LOG_INIT("esx.esx_vi_types"); return -1; \ } \ \ - switch (type) { \ + switch ((int)type) { \ _dispatch \ \ case esxVI_Type_##__type: \ @@ -967,7 +967,7 @@ esxVI_AnyType_DeepCopy(esxVI_AnyType **dest, esxVI_AnyType *src) goto failure; } - switch (src->type) { + switch ((int)src->type) { case esxVI_Type_Boolean: (*dest)->boolean = src->boolean; break; @@ -1071,7 +1071,7 @@ esxVI_AnyType_Deserialize(xmlNodePtr node, esxVI_AnyType **anyType) (*anyType)->_name = number; \ } while (0) - switch ((*anyType)->type) { + switch ((int)(*anyType)->type) { case esxVI_Type_Boolean: if (STREQ((*anyType)->value, "true")) { (*anyType)->boolean = esxVI_Boolean_True; @@ -1876,6 +1876,7 @@ esxVI_VirtualMachinePowerState_ConvertToLibvirt case esxVI_VirtualMachinePowerState_Suspended: return VIR_DOMAIN_PAUSED; + case esxVI_VirtualMachinePowerState_Undefined: default: return VIR_DOMAIN_NOSTATE; } -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements, or explicitly cast away enum type where we don't want to list all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/esx/esx_driver.c | 1 + src/esx/esx_vi.c | 11 +++++++---- src/esx/esx_vi_types.c | 9 +++++---- 3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index f575362059..c5f04efa81 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3603,6 +3603,7 @@ esxDomainGetSchedulerParametersFlags(virDomainPtr domain, params[i].value.i = -3; break;
+ case esxVI_SharesLevel_Undefined: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Shares level has unknown value %d"),
Shouldn't this be: virReportEnumRangeError(esxVI_SharesLevel, sharesInfo->level);
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index edf52ff828..103f726069 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -483,7 +483,7 @@ esxVI_SharedCURL_Lock(CURL *handle ATTRIBUTE_UNUSED, curl_lock_data data, size_t i; esxVI_SharedCURL *shared = userptr;
- switch (data) { + switch ((int)data) { case CURL_LOCK_DATA_SHARE: i = 0; break; @@ -511,7 +511,7 @@ esxVI_SharedCURL_Unlock(CURL *handle ATTRIBUTE_UNUSED, curl_lock_data data, size_t i; esxVI_SharedCURL *shared = userptr;
- switch (data) { + switch ((int)data) { case CURL_LOCK_DATA_SHARE: i = 0; break; @@ -1563,6 +1563,7 @@ esxVI_Context_Execute(esxVI_Context *ctx, const char *methodName,
break;
+ case esxVI_Occurrence_Undefined: default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument (occurrence)"));
and: virReportEnumRangeError(esxVI_Occurrence, occurrence);
@@ -2280,9 +2281,11 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx, type, root->type); break;
+ case esxVI_Occurrence_None: + case esxVI_Occurrence_Undefined: default: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Invalid occurrence value")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid occurrence value %d"), occurrence);
and: virReportEnumRangeError(esxVI_Occurrence, occurrence);
break; }
diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c index be35af861c..74183f8307 100644 --- a/src/esx/esx_vi_types.c +++ b/ @@ -544,7 +544,7 @@ VIR_LOG_INIT("esx.esx_vi_types");
#define ESX_VI__TEMPLATE__DISPATCH(_actual_type, _actual_type_name, __type, \ _dispatch, _error_return) \ - switch (_actual_type) { \ + switch ((int)_actual_type) { \ _dispatch \ \ case esxVI_Type_##__type: \ @@ -690,7 +690,7 @@ VIR_LOG_INIT("esx.esx_vi_types"); return -1; \ } \ \ - switch (type) { \ + switch ((int)type) { \ _dispatch \ \ case esxVI_Type_##__type: \ @@ -967,7 +967,7 @@ esxVI_AnyType_DeepCopy(esxVI_AnyType **dest, esxVI_AnyType *src) goto failure; }
- switch (src->type) { + switch ((int)src->type) { case esxVI_Type_Boolean: (*dest)->boolean = src->boolean; break; @@ -1071,7 +1071,7 @@ esxVI_AnyType_Deserialize(xmlNodePtr node, esxVI_AnyType **anyType) (*anyType)->_name = number; \ } while (0)
- switch ((*anyType)->type) { + switch ((int)(*anyType)->type) { case esxVI_Type_Boolean: if (STREQ((*anyType)->value, "true")) { (*anyType)->boolean = esxVI_Boolean_True; @@ -1876,6 +1876,7 @@ esxVI_VirtualMachinePowerState_ConvertToLibvirt case esxVI_VirtualMachinePowerState_Suspended: return VIR_DOMAIN_PAUSED;
+ case esxVI_VirtualMachinePowerState_Undefined:
Fix indent - off by 2. Reviewed-by: John Ferlan <jferlan@redhat.com> John
default: return VIR_DOMAIN_NOSTATE; }
On Tue, Feb 20, 2018 at 02:03:19PM -0500, John Ferlan wrote:
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements, or explicitly cast away enum type where we don't want to list all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/esx/esx_driver.c | 1 + src/esx/esx_vi.c | 11 +++++++---- src/esx/esx_vi_types.c | 9 +++++---- 3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index f575362059..c5f04efa81 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3603,6 +3603,7 @@ esxDomainGetSchedulerParametersFlags(virDomainPtr domain, params[i].value.i = -3; break;
+ case esxVI_SharesLevel_Undefined: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Shares level has unknown value %d"),
Shouldn't this be:
virReportEnumRangeError(esxVI_SharesLevel, sharesInfo->level);
Yeah, I had only changed messages I added in v1, but thse can be touched up too.
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index edf52ff828..103f726069 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -483,7 +483,7 @@ esxVI_SharedCURL_Lock(CURL *handle ATTRIBUTE_UNUSED, curl_lock_data data, size_t i; esxVI_SharedCURL *shared = userptr;
- switch (data) { + switch ((int)data) { case CURL_LOCK_DATA_SHARE: i = 0; break; @@ -511,7 +511,7 @@ esxVI_SharedCURL_Unlock(CURL *handle ATTRIBUTE_UNUSED, curl_lock_data data, size_t i; esxVI_SharedCURL *shared = userptr;
- switch (data) { + switch ((int)data) { case CURL_LOCK_DATA_SHARE: i = 0; break; @@ -1563,6 +1563,7 @@ esxVI_Context_Execute(esxVI_Context *ctx, const char *methodName,
break;
+ case esxVI_Occurrence_Undefined: default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument (occurrence)"));
and:
virReportEnumRangeError(esxVI_Occurrence, occurrence);
@@ -2280,9 +2281,11 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx, type, root->type); break;
+ case esxVI_Occurrence_None: + case esxVI_Occurrence_Undefined: default: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Invalid occurrence value")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid occurrence value %d"), occurrence);
and:
virReportEnumRangeError(esxVI_Occurrence, occurrence);
break; }
diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c index be35af861c..74183f8307 100644 --- a/src/esx/esx_vi_types.c +++ b/ @@ -544,7 +544,7 @@ VIR_LOG_INIT("esx.esx_vi_types");
#define ESX_VI__TEMPLATE__DISPATCH(_actual_type, _actual_type_name, __type, \ _dispatch, _error_return) \ - switch (_actual_type) { \ + switch ((int)_actual_type) { \ _dispatch \ \ case esxVI_Type_##__type: \ @@ -690,7 +690,7 @@ VIR_LOG_INIT("esx.esx_vi_types"); return -1; \ } \ \ - switch (type) { \ + switch ((int)type) { \ _dispatch \ \ case esxVI_Type_##__type: \ @@ -967,7 +967,7 @@ esxVI_AnyType_DeepCopy(esxVI_AnyType **dest, esxVI_AnyType *src) goto failure; }
- switch (src->type) { + switch ((int)src->type) { case esxVI_Type_Boolean: (*dest)->boolean = src->boolean; break; @@ -1071,7 +1071,7 @@ esxVI_AnyType_Deserialize(xmlNodePtr node, esxVI_AnyType **anyType) (*anyType)->_name = number; \ } while (0)
- switch ((*anyType)->type) { + switch ((int)(*anyType)->type) { case esxVI_Type_Boolean: if (STREQ((*anyType)->value, "true")) { (*anyType)->boolean = esxVI_Boolean_True; @@ -1876,6 +1876,7 @@ esxVI_VirtualMachinePowerState_ConvertToLibvirt case esxVI_VirtualMachinePowerState_Suspended: return VIR_DOMAIN_PAUSED;
+ case esxVI_VirtualMachinePowerState_Undefined:
Fix indent - off by 2.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
default: return VIR_DOMAIN_NOSTATE; }
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Ensure all enum cases are listed in switch statements. This improves debug logging integration with openwsman. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hyperv/hyperv_driver.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index ee94fd3511..e512b626ea 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1656,13 +1656,27 @@ hypervDebugHandler(const char *message, debug_level_e level, switch (level) { case DEBUG_LEVEL_ERROR: case DEBUG_LEVEL_CRITICAL: - VIR_ERROR(_("openwsman error: %s"), message); + case DEBUG_LEVEL_ALWAYS: + VIR_ERROR(_("openwsman: %s"), message); break; case DEBUG_LEVEL_WARNING: - VIR_WARN("openwsman warning: %s", message); + VIR_WARN("openwsman: %s", message); break; + case DEBUG_LEVEL_MESSAGE: + VIR_INFO("openwsman: %s", message); + break; + + case DEBUG_LEVEL_INFO: + VIR_INFO("openwsman: %s", message); + break; + + case DEBUG_LEVEL_DEBUG: + VIR_DEBUG("openwsman: %s", message); + break; + + case DEBUG_LEVEL_NONE: default: /* Ignore the rest */ break; -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements. This improves debug logging integration with openwsman.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hyperv/hyperv_driver.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Cast away enum type for libxl schedular constants since we don't want to cover all of them and don't want build to break when new ones are added. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index be11134fb2..4b52de36f5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4497,7 +4497,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams) if (nparams) *nparams = 0; - switch (sched_id) { + switch ((int)sched_id) { case LIBXL_SCHEDULER_SEDF: name = "sedf"; break; -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Cast away enum type for libxl schedular constants since we don't want to cover all of them and don't want build to break when new ones are added.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, Feb 20, 2018 at 05:08:14PM +0000, Daniel P. Berrangé wrote:
Cast away enum type for libxl schedular constants since we don't want to
s/schedular/scheduler/
cover all of them and don't want build to break when new ones are added.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index be11134fb2..4b52de36f5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4497,7 +4497,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
if (nparams) *nparams = 0; - switch (sched_id) { + switch ((int)sched_id) {
We should get consistent with the spacing after cast and add it to HACKING. This way I can't tell you to add a space there =)
case LIBXL_SCHEDULER_SEDF: name = "sedf"; break; -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Feb 21, 2018 at 10:35:04AM +0100, Martin Kletzander wrote:
On Tue, Feb 20, 2018 at 05:08:14PM +0000, Daniel P. Berrangé wrote:
Cast away enum type for libxl schedular constants since we don't want to
s/schedular/scheduler/
cover all of them and don't want build to break when new ones are added.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index be11134fb2..4b52de36f5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4497,7 +4497,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
if (nparams) *nparams = 0; - switch (sched_id) { + switch ((int)sched_id) {
We should get consistent with the spacing after cast and add it to HACKING. This way I can't tell you to add a space there =)
Adding a space between a typecast and a variable is a-typical in general, so I don't see why we should do it for switch() statements.
case LIBXL_SCHEDULER_SEDF: name = "sedf"; break; -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Feb 21, 2018 at 09:39:07AM +0000, Daniel P. Berrangé wrote:
On Wed, Feb 21, 2018 at 10:35:04AM +0100, Martin Kletzander wrote:
On Tue, Feb 20, 2018 at 05:08:14PM +0000, Daniel P. Berrangé wrote:
Cast away enum type for libxl schedular constants since we don't want to
s/schedular/scheduler/
cover all of them and don't want build to break when new ones are added.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index be11134fb2..4b52de36f5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4497,7 +4497,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
if (nparams) *nparams = 0; - switch (sched_id) { + switch ((int)sched_id) {
We should get consistent with the spacing after cast and add it to HACKING. This way I can't tell you to add a space there =)
Adding a space between a typecast and a variable is a-typical in general, so I don't see why we should do it for switch() statements.
Well, I didn't use to do it, but nowadays I do since it's more clearly readable. Plus it looks like majority casts in our code use spaces. I'll try create a syntax-check and make some statistics, we'll see if that works or not. I didn't mean to pollute this series with unrelated discussion.
case LIBXL_SCHEDULER_SEDF: name = "sedf"; break; -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_container.c | 8 ++++---- src/lxc/lxc_controller.c | 8 +++++++- src/lxc/lxc_driver.c | 38 ++++++++++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 96fceaf1b8..14928e8ece 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2035,7 +2035,7 @@ static int lxcContainerDropCapabilities(virDomainDefPtr def, break; case VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT: - switch ((virDomainCapsFeature) i) { + switch (i) { case VIR_DOMAIN_CAPS_FEATURE_SYS_BOOT: /* No use of reboot */ toDrop = !keepReboot && (state != VIR_TRISTATE_SWITCH_ON); break; @@ -2066,10 +2066,10 @@ static int lxcContainerDropCapabilities(virDomainDefPtr def, } break; + case VIR_DOMAIN_CAPABILITIES_POLICY_LAST: default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported capabilities policy: %s"), - virDomainCapabilitiesPolicyTypeToString(policy)); + virReportEnumRangeError(virDomainCapabilitiesPolicy, policy); + return -1; } } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c5e67df938..7346804c18 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -395,8 +395,14 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl) case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_HOSTDEV: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported net type %s"), + virDomainNetTypeToString(ctrl->def->nets[i]->type)); + goto cleanup; + case VIR_DOMAIN_NET_TYPE_LAST: default: - break; + virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type); + goto cleanup; } } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 961baa344c..3d5b2254f2 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3968,10 +3968,21 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, if (!(veth = virLXCProcessSetupInterfaceDirect(conn, vm->def, net))) goto cleanup; } break; - default: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Network device type is not supported")); goto cleanup; + case VIR_DOMAIN_NET_TYPE_LAST: + default: + virReportEnumRangeError(virDomainNetType, actualType); + goto cleanup; } /* Set bandwidth or warn if requested and not supported. */ actualBandwidth = virDomainNetGetActualBandwidth(net); @@ -4011,6 +4022,15 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, ignore_value(virNetDevMacVLanDelete(veth)); break; + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: default: /* no-op */ break; @@ -4446,13 +4466,23 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, * the host side. Further the container can change * the mac address of NIC name, so we can't easily * find out which guest NIC it maps to + */ case VIR_DOMAIN_NET_TYPE_DIRECT: - */ - - default: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only bridged veth devices can be detached")); goto cleanup; + case VIR_DOMAIN_NET_TYPE_LAST: + default: + virReportEnumRangeError(virDomainNetType, actualType); + goto cleanup; } virDomainAuditNet(vm, detach, NULL, "detach", true); -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_container.c | 8 ++++---- src/lxc/lxc_controller.c | 8 +++++++- src/lxc/lxc_driver.c | 38 ++++++++++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 9 deletions(-)
[...]
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c5e67df938..7346804c18 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -395,8 +395,14 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl) case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_HOSTDEV: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported net type %s"), + virDomainNetTypeToString(ctrl->def->nets[i]->type)); + goto cleanup;
So this will cause an error; whereas, previously there was no error for main startup and the values essentially ignored (with nothing added to nicindexes). Is this the "safe" thing to do? Especially for existing configs which may now fail... Perhaps some kind of VIR_* message printed instead? This certainly falls into the category of probably should have been an error all along, but "fear" (at least in my mind) causes trepidation to just spring an error on someone now. Feels like one of those guest disappearing things, but you understand the LXC controller better than I - so I'll just note it and let you decide.
+ case VIR_DOMAIN_NET_TYPE_LAST: default: - break; + virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type); + goto cleanup;
I think having an error here is certainly reasonable even though it probably is a can't get there type thing.
} }
[...] Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, Feb 20, 2018 at 02:33:56PM -0500, John Ferlan wrote:
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_container.c | 8 ++++---- src/lxc/lxc_controller.c | 8 +++++++- src/lxc/lxc_driver.c | 38 ++++++++++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 9 deletions(-)
[...]
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c5e67df938..7346804c18 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -395,8 +395,14 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl) case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_HOSTDEV: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported net type %s"), + virDomainNetTypeToString(ctrl->def->nets[i]->type)); + goto cleanup;
So this will cause an error; whereas, previously there was no error for main startup and the values essentially ignored (with nothing added to nicindexes). Is this the "safe" thing to do? Especially for existing configs which may now fail... Perhaps some kind of VIR_* message printed instead?
This certainly falls into the category of probably should have been an error all along, but "fear" (at least in my mind) causes trepidation to just spring an error on someone now.
Feels like one of those guest disappearing things, but you understand the LXC controller better than I - so I'll just note it and let you decide.
LXC controller code only runs on guest startup, so reporting an error here is good. The driver code should not have even launched the controller if it saw these NIC types.
+ case VIR_DOMAIN_NET_TYPE_LAST: default: - break; + virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type); + goto cleanup;
I think having an error here is certainly reasonable even though it probably is a can't get there type thing.
} }
[...]
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_conf.h | 4 ++-- src/nwfilter/nwfilter_ebiptables_driver.c | 17 +++++++++++------ src/nwfilter/nwfilter_learnipaddr.c | 6 +++++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index eeec10e67b..f960bf3d56 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -86,7 +86,7 @@ typedef enum { (((data)->flags) & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) /* datatypes appearing in rule attributes */ -enum attrDatatype { +typedef enum attrDatatype { DATATYPE_UINT16 = (1 << 0), DATATYPE_UINT8 = (1 << 1), DATATYPE_UINT16_HEX = (1 << 2), @@ -106,7 +106,7 @@ enum attrDatatype { DATATYPE_IPSETFLAGS = (1 << 16), DATATYPE_LAST = (1 << 17), -}; +} virNWFilterAttrDataType; # define NWFILTER_MAC_BGA "01:80:c2:00:00:00" diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index b8682a1130..b19b07c845 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -320,11 +320,16 @@ _printDataType(virNWFilterVarCombIterPtr vars, VIR_FREE(flags); break; - default: + case DATATYPE_STRING: + case DATATYPE_STRINGCOPY: + case DATATYPE_BOOLEAN: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unhandled datatype %x"), item->datatype); + _("Cannot print data type %x"), item->datatype); + return -1; + case DATATYPE_LAST: + default: + virReportEnumRangeError(virNWFilterAttrDataType, item->datatype); return -1; - break; } return 0; @@ -1183,7 +1188,7 @@ _iptablesCreateRuleInstance(virFirewallPtr fw, PRINT_IPT_ROOT_CHAIN(chain, chainPrefix, ifname); - switch (rule->prtclType) { + switch ((int)rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_TCP: case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: fwrule = virFirewallAddRule(fw, layer, @@ -1873,7 +1878,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, #define INST_ITEM_MASK(S, I, MASK, C) \ INST_ITEM_2PARMS(S, I, MASK, C, "/") - switch (rule->prtclType) { + switch ((int)rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_MAC: fwrule = virFirewallAddRule(fw, VIR_FIREWALL_LAYER_ETHERNET, "-t", "nat", @@ -2677,7 +2682,7 @@ ebtablesCreateTmpSubChainFW(virFirewallPtr fw, fwrule = virFirewallAddRule(fw, VIR_FIREWALL_LAYER_ETHERNET, "-t", "nat", "-A", rootchain, NULL); - switch (protoidx) { + switch ((int)protoidx) { case L2_PROTO_MAC_IDX: break; case L2_PROTO_STP_IDX: diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 5b95f0e613..9ca0639576 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -430,7 +430,7 @@ learnIPAddressThread(void *arg) } virBufferAddLit(&buf, "src port 67 and dst port 68"); break; - default: + case DETECT_STATIC: if (techdriver->applyBasicRules(req->ifname, &req->macaddr) < 0) { req->status = EINVAL; @@ -438,6 +438,10 @@ learnIPAddressThread(void *arg) } virBufferAsprintf(&buf, "ether host %s or ether dst ff:ff:ff:ff:ff:ff", macaddr); + break; + default: + req->status = EINVAL; + goto done; } if (virBufferError(&buf)) { -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_conf.h | 4 ++-- src/nwfilter/nwfilter_ebiptables_driver.c | 17 +++++++++++------ src/nwfilter/nwfilter_learnipaddr.c | 6 +++++- 3 files changed, 18 insertions(+), 9 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 26 ++++++++++++++++++-------- src/qemu/qemu_domain.c | 21 +++++++++++++++++++++ src/qemu/qemu_driver.c | 25 +++++++++++++++++-------- src/qemu/qemu_hotplug.c | 33 ++++++++++++++++++++++++++++----- src/qemu/qemu_migration.c | 11 ++++++++++- src/qemu/qemu_process.c | 2 ++ 6 files changed, 96 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 34dda23288..fa0aa5d5c3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2632,7 +2632,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: switch ((virDomainControllerModelSCSI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: - switch ((virDomainDeviceAddressType) address_type) { + switch (address_type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: virBufferAddLit(&buf, "virtio-scsi-ccw"); break; @@ -2684,7 +2684,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: - switch ((virDomainDeviceAddressType) address_type) { + switch (address_type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: virBufferAddLit(&buf, "virtio-serial-pci"); break; @@ -3407,12 +3407,17 @@ qemuBuildNicDevStr(virDomainDefPtr def, case VIR_DOMAIN_NET_VIRTIO_TX_MODE_TIMER: virBufferAddLit(&buf, "timer"); break; + + case VIR_DOMAIN_NET_VIRTIO_TX_MODE_DEFAULT: + break; + + case VIR_DOMAIN_NET_VIRTIO_TX_MODE_LAST: default: /* this should never happen, if it does, we need * to add another case to this switch. */ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unrecognized virtio-net-pci 'tx' option")); + virReportEnumRangeError(virDomainNetVirtioTxModeType, + net->driver.virtio.txmode); goto error; } } else { @@ -6598,7 +6603,7 @@ qemuBuildGlobalControllerCommandLine(virCommandPtr cmd, bool cap = false; bool machine = false; - switch ((virDomainControllerModelPCI) cont->model) { + switch (cont->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: hoststr = "i440FX-pcihost"; cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_I440FX_PCI_HOLE64_SIZE); @@ -6941,7 +6946,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, if (cpu_flags && !cpu) { const char *default_model; - switch (def->os.arch) { + switch ((int)def->os.arch) { case VIR_ARCH_I686: default_model = "qemu32"; break; @@ -6987,7 +6992,7 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd, bool disableKVM = false; bool enableKVM = false; - switch (def->virtType) { + switch ((int)def->virtType) { case VIR_DOMAIN_VIRT_QEMU: if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) disableKVM = true; @@ -7955,8 +7960,13 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: virBufferAddLit(&opt, "agent-mouse=on,"); break; - default: + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT: break; + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsSpiceMouseMode, + graphics->data.spice.mousemode); + goto error; } } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b1308e5a49..8b4efc82de 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2804,6 +2804,27 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, addPCIRoot = true; break; + case VIR_ARCH_ARMV6L: + case VIR_ARCH_ARMV7B: + case VIR_ARCH_CRIS: + case VIR_ARCH_ITANIUM: + case VIR_ARCH_LM32: + case VIR_ARCH_M68K: + case VIR_ARCH_MICROBLAZE: + case VIR_ARCH_MICROBLAZEEL: + case VIR_ARCH_MIPS: + case VIR_ARCH_MIPSEL: + case VIR_ARCH_MIPS64: + case VIR_ARCH_MIPS64EL: + case VIR_ARCH_OR32: + case VIR_ARCH_PARISC: + case VIR_ARCH_PARISC64: + case VIR_ARCH_PPCLE: + case VIR_ARCH_UNICORE32: + case VIR_ARCH_XTENSA: + case VIR_ARCH_XTENSAEB: + case VIR_ARCH_NONE: + case VIR_ARCH_LAST: default: break; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 416567512d..8aa293bf1b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3016,13 +3016,8 @@ qemuCompressGetCommand(virQEMUSaveFormat compression) ret = virCommandNew(prog); virCommandAddArg(ret, "-dc"); - switch (compression) { - case QEMU_SAVE_FORMAT_LZOP: + if (compression == QEMU_SAVE_FORMAT_LZOP) virCommandAddArg(ret, "--ignore-warn"); - break; - default: - break; - } return ret; } @@ -17792,11 +17787,18 @@ qemuDomainOpenGraphics(virDomainPtr dom, case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: protocol = "spice"; break; - default: + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Can only open VNC or SPICE graphics backends, not %s"), virDomainGraphicsTypeToString(vm->def->graphics[idx]->type)); goto endjob; + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsType, + vm->def->graphics[idx]->type); + goto endjob; } if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0) @@ -17856,11 +17858,18 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: protocol = "spice"; break; - default: + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Can only open VNC or SPICE graphics backends, not %s"), virDomainGraphicsTypeToString(vm->def->graphics[idx]->type)); goto cleanup; + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsType, + vm->def->graphics[idx]->type); + goto cleanup; } if (qemuSecuritySetSocketLabel(driver->securityManager, vm->def) < 0) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 04f4f689a9..f28006e3cd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2978,11 +2978,24 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm, case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: break; - default: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("filters not supported on interfaces of type %s"), virDomainNetTypeToString(virDomainNetGetActualType(newdev))); return -1; + case VIR_DOMAIN_NET_TYPE_LAST: + default: + virReportEnumRangeError(virDomainNetType, + virDomainNetGetActualType(newdev)); + return -1; } virDomainConfNWFilterTeardown(olddev); @@ -3277,12 +3290,16 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, /* all handled in common code directly below this switch */ break; - default: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("unable to change config on '%s' network type"), virDomainNetTypeToString(newdev->type)); - break; - + goto cleanup; + case VIR_DOMAIN_NET_TYPE_LAST: + default: + virReportEnumRangeError(virDomainNetType, newdev->type); + goto cleanup; } } else { /* interface type has changed. There are a few special cases @@ -3661,10 +3678,16 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver, } break; - default: + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to change config on '%s' graphics type"), type); break; + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsType, dev->type); + break; } cleanup: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 99d58325cb..2d92d38936 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1468,11 +1468,20 @@ qemuMigrationJobName(virDomainObjPtr vm) switch (priv->job.asyncJob) { case QEMU_ASYNC_JOB_MIGRATION_OUT: - return _("migration job"); + return _("migration out job"); case QEMU_ASYNC_JOB_SAVE: return _("domain save job"); case QEMU_ASYNC_JOB_DUMP: return _("domain core dump job"); + case QEMU_ASYNC_JOB_NONE: + return _("no job"); + case QEMU_ASYNC_JOB_MIGRATION_IN: + return _("migration in job"); + case QEMU_ASYNC_JOB_SNAPSHOT: + return _("snapshot job"); + case QEMU_ASYNC_JOB_START: + return _("start job"); + case QEMU_ASYNC_JOB_LAST: default: return _("job"); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 13b05d236a..dc33eb7bcd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -679,6 +679,8 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED, detail = VIR_DOMAIN_EVENT_SHUTDOWN_HOST; break; + case VIR_TRISTATE_BOOL_ABSENT: + case VIR_TRISTATE_BOOL_LAST: default: detail = VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED; break; -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 26 ++++++++++++++++++-------- src/qemu/qemu_domain.c | 21 +++++++++++++++++++++ src/qemu/qemu_driver.c | 25 +++++++++++++++++-------- src/qemu/qemu_hotplug.c | 33 ++++++++++++++++++++++++++++----- src/qemu/qemu_migration.c | 11 ++++++++++- src/qemu/qemu_process.c | 2 ++ 6 files changed, 96 insertions(+), 22 deletions(-)
[...]
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 99d58325cb..2d92d38936 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1468,11 +1468,20 @@ qemuMigrationJobName(virDomainObjPtr vm)
switch (priv->job.asyncJob) { case QEMU_ASYNC_JOB_MIGRATION_OUT: - return _("migration job"); + return _("migration out job"); case QEMU_ASYNC_JOB_SAVE: return _("domain save job"); case QEMU_ASYNC_JOB_DUMP: return _("domain core dump job"); + case QEMU_ASYNC_JOB_NONE: + return _("no job");
Note from previous review applies... Can this be "undefined" instead of "no"? Looking at the caller you could get output such as "operation failed: no job: %s" where %s could be "is not active", "unexpectedly failed", "canceled by client", or "failed due to I/O error" Could also just lump it in with LAST and default.
+ case QEMU_ASYNC_JOB_MIGRATION_IN: + return _("migration in job"); + case QEMU_ASYNC_JOB_SNAPSHOT: + return _("snapshot job"); + case QEMU_ASYNC_JOB_START: + return _("start job"); + case QEMU_ASYNC_JOB_LAST: default: return _("job"); }
[...] Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, Feb 20, 2018 at 03:28:56PM -0500, John Ferlan wrote:
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements, or cast away enum type in places where we don't wish to cover all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 26 ++++++++++++++++++-------- src/qemu/qemu_domain.c | 21 +++++++++++++++++++++ src/qemu/qemu_driver.c | 25 +++++++++++++++++-------- src/qemu/qemu_hotplug.c | 33 ++++++++++++++++++++++++++++----- src/qemu/qemu_migration.c | 11 ++++++++++- src/qemu/qemu_process.c | 2 ++ 6 files changed, 96 insertions(+), 22 deletions(-)
[...]
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 99d58325cb..2d92d38936 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1468,11 +1468,20 @@ qemuMigrationJobName(virDomainObjPtr vm)
switch (priv->job.asyncJob) { case QEMU_ASYNC_JOB_MIGRATION_OUT: - return _("migration job"); + return _("migration out job"); case QEMU_ASYNC_JOB_SAVE: return _("domain save job"); case QEMU_ASYNC_JOB_DUMP: return _("domain core dump job"); + case QEMU_ASYNC_JOB_NONE: + return _("no job");
Note from previous review applies...
Can this be "undefined" instead of "no"? Looking at the caller you could get output such as "operation failed: no job: %s" where %s could be "is not active", "unexpectedly failed", "canceled by client", or "failed due to I/O error"
Could also just lump it in with LAST and default.
Ok, yes.
+ case QEMU_ASYNC_JOB_MIGRATION_IN: + return _("migration in job"); + case QEMU_ASYNC_JOB_SNAPSHOT: + return _("snapshot job"); + case QEMU_ASYNC_JOB_START: + return _("start job"); + case QEMU_ASYNC_JOB_LAST: default: return _("job"); }
[...]
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Ensure all enum cases are listed in switch statements. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetclient.c | 2 ++ src/rpc/virnetclientprogram.c | 1 + src/rpc/virnetserverprogram.c | 4 ++++ 3 files changed, 7 insertions(+) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 8aeacf8774..0c8d58c32c 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -1287,6 +1287,8 @@ virNetClientCallDispatch(virNetClientPtr client) case VIR_NET_STREAM_HOLE: /* Sparse stream protocol*/ return virNetClientCallDispatchStream(client); + case VIR_NET_CALL: + case VIR_NET_CALL_WITH_FDS: default: virReportError(VIR_ERR_RPC, _("got unexpected RPC call prog %d vers %d proc %d type %d"), diff --git a/src/rpc/virnetclientprogram.c b/src/rpc/virnetclientprogram.c index d81a055424..505b40fc4b 100644 --- a/src/rpc/virnetclientprogram.c +++ b/src/rpc/virnetclientprogram.c @@ -384,6 +384,7 @@ int virNetClientProgramCall(virNetClientProgramPtr prog, virNetClientProgramDispatchError(prog, msg); goto error; + case VIR_NET_CONTINUE: default: virReportError(VIR_ERR_RPC, _("Unexpected message status %d"), msg->header.status); diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 557651ffbd..75b0052cdb 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -324,6 +324,10 @@ int virNetServerProgramDispatch(virNetServerProgramPtr prog, ret = 0; break; + case VIR_NET_REPLY: + case VIR_NET_REPLY_WITH_FDS: + case VIR_NET_MESSAGE: + case VIR_NET_STREAM_HOLE: default: virReportError(VIR_ERR_RPC, _("Unexpected message type %u"), -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetclient.c | 2 ++ src/rpc/virnetclientprogram.c | 1 + src/rpc/virnetserverprogram.c | 4 ++++ 3 files changed, 7 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Ensure all enum cases are listed in switch statements. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 4800d5255a..a845dc7995 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -82,6 +82,7 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name, } break; + case SECURITY_DRIVER_ERROR: default: return NULL; } -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_driver.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Ensure all enum cases are listed in switch statements. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/vmx/vmx.c | 26 ++++++++++++++++++++++++-- src/xenconfig/xen_common.c | 18 +++++++++++++++--- src/xenconfig/xen_xl.c | 7 ++++++- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 1d13ab03db..3b0c16d5a6 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -3283,11 +3283,19 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe break; - default: + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported graphics type '%s'"), virDomainGraphicsTypeToString(def->graphics[i]->type)); goto cleanup; + + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsType, def->graphics[i]->type); + goto cleanup; } } @@ -3782,10 +3790,24 @@ virVMXFormatEthernet(virDomainNetDefPtr def, int controller, controller); break; - default: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported net type '%s'"), virDomainNetTypeToString(def->type)); return -1; + + case VIR_DOMAIN_NET_TYPE_LAST: + default: + virReportEnumRangeError(virDomainNetType, def->type); + return -1; } /* def:mac -> vmx:addressType, vmx:(generated)Address, vmx:checkMACAddress */ diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index ca3b4dee6a..bc43185363 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -1258,10 +1258,22 @@ xenFormatNet(virConnectPtr conn, } break; + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_USER: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported net type '%s'"), + virDomainNetTypeToString(net->type)); + goto cleanup; + + case VIR_DOMAIN_NET_TYPE_LAST: default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported network type %d"), - net->type); + virReportEnumRangeError(virDomainNetType, net->type); goto cleanup; } diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 2ef80eb83a..e1ec8e765f 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -1641,8 +1641,13 @@ xenFormatXLSpice(virConfPtr conf, virDomainDefPtr def) if (xenConfigSetInt(conf, "spicevdagent", 1) < 0) return -1; break; - default: + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT: break; + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_LAST: + default: + virReportEnumRangeError(virDomainGraphicsSpiceMouseMode, + graphics->data.spice.mousemode); + return -1; } } -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Ensure all enum cases are listed in switch statements.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/vmx/vmx.c | 26 ++++++++++++++++++++++++-- src/xenconfig/xen_common.c | 18 +++++++++++++++--- src/xenconfig/xen_xl.c | 7 ++++++- 3 files changed, 45 insertions(+), 6 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Cast away enum type in places where we don't wish to cover all cases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virt-host-validate-qemu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index 2aa396e3ab..d7573ea8b3 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -33,13 +33,14 @@ int virHostValidateQEMU(void) int ret = 0; bool hasHwVirt = false; bool hasVirtFlag = false; + virArch arch = virArchFromHost(); const char *kvmhint = _("Check that CPU and firmware supports virtualization " "and kvm module is loaded"); if (!(flags = virHostValidateGetCPUFlags())) return -1; - switch (virArchFromHost()) { + switch ((int)arch) { case VIR_ARCH_I686: case VIR_ARCH_X86_64: hasVirtFlag = true; -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
Cast away enum type in places where we don't wish to cover all cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virt-host-validate-qemu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
As a general rule any time we switch() on something that is an enum, we want to have a case for every enum constant. The -Wswitch warning will report any switch where we've violated this rule, except if that switch has a default case. Unfortunately it is reasonable to want to list all enum constants *and* also have a default case. To get a warning in that scenario requires that we turn on -Wswitch-enum. In a few cases where we explicitly don't want to list all enum cases, we can discard the enum type checking by casting the value to a plain int. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- m4/virt-compile-warnings.m4 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 4b35a6f6b5..fc185aef38 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -47,8 +47,6 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ dontwarn="$dontwarn -Wlong-long" # We allow manual list of all enum cases without default: dontwarn="$dontwarn -Wswitch-default" - # We allow optional default: instead of listing all enum values - dontwarn="$dontwarn -Wswitch-enum" # Not a problem since we don't use -fstrict-overflow dontwarn="$dontwarn -Wstrict-overflow" # Not a problem since we don't use -funsafe-loop-optimizations @@ -184,6 +182,11 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # that one off, so we need to manually enable this again wantwarn="$wantwarn -Wjump-misses-init" + # GNULIB explicitly filters it out, preferring -Wswitch + # but that doesn't report missing enums if a default: + # is present. + wantwarn="$wantwarn -Wswitch-enum" + # GNULIB turns on -Wformat=2 which implies -Wformat-nonliteral, # so we need to manually re-exclude it. Also, older gcc 4.2 # added an implied ATTRIBUTE_NONNULL on any parameter marked -- 2.14.3
On 02/20/2018 12:08 PM, Daniel P. Berrangé wrote:
As a general rule any time we switch() on something that is an enum, we want to have a case for every enum constant. The -Wswitch warning will report any switch where we've violated this rule, except if that switch has a default case.
Unfortunately it is reasonable to want to list all enum constants *and* also have a default case. To get a warning in that scenario requires that we turn on -Wswitch-enum.
In a few cases where we explicitly don't want to list all enum cases, we can discard the enum type checking by casting the value to a plain int.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- m4/virt-compile-warnings.m4 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Martin Kletzander