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@redhat.com>
---
src/libvirt.c | 3 +++
src/util/vircgroup.c | 1 +
src/util/vircrypto.c | 2 ++
src/util/virerror.c | 6 +++++-
src/util/virfdstream.c | 4 ++++
src/util/virfirewall.c | 1 +
src/util/virhook.c | 12 +++++------
src/util/virhostdev.c | 8 +++++++-
src/util/virhostmem.c | 5 +++++
src/util/virjson.c | 5 +++++
src/util/virlog.c | 7 +++++--
src/util/virnetdev.c | 1 +
src/util/virnetdevmacvlan.c | 3 +++
src/util/virnetdevvportprofile.c | 43 ++++++++++++++++++++++++++++++++++------
src/util/virnuma.c | 5 ++++-
src/util/virprocess.c | 1 +
src/util/virqemu.c | 5 +++++
src/util/virsexpr.c | 2 ++
src/util/virsocketaddr.c | 13 +++++++-----
src/util/virstoragefile.c | 15 ++++++++++++--
20 files changed, 118 insertions(+), 24 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 536d56f0a5..6eb265c2d7 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -503,6 +503,9 @@ DllMain(HINSTANCE instance ATTRIBUTE_UNUSED,
since (hopefully) windows cleans up
everything on process exit */
break;
+
+ default:
+ break;
}
return TRUE;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 0a31947b0d..1b256af92f 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1495,6 +1495,7 @@ virCgroupNewThread(virCgroupPtr domain,
goto cleanup;
break;
case VIR_CGROUP_THREAD_LAST:
+ default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected name value %d"), nameval);
goto cleanup;
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 48b04fc8ce..dbbe2f7fd5 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -112,6 +112,7 @@ virCryptoHaveCipher(virCryptoCipher algorithm)
case VIR_CRYPTO_CIPHER_NONE:
case VIR_CRYPTO_CIPHER_LAST:
+ default:
break;
};
@@ -263,6 +264,7 @@ virCryptoEncryptData(virCryptoCipher algorithm,
case VIR_CRYPTO_CIPHER_NONE:
case VIR_CRYPTO_CIPHER_LAST:
+ default:
break;
}
diff --git a/src/util/virerror.c b/src/util/virerror.c
index c000b00436..35f819e4e4 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -51,9 +51,9 @@ static virLogPriority virErrorLevelPriority(virErrorLevel level)
case VIR_ERR_WARNING:
return VIR_LOG_WARN;
case VIR_ERR_ERROR:
+ default:
return VIR_LOG_ERROR;
}
- return VIR_LOG_ERROR;
}
@@ -616,6 +616,7 @@ virDefaultErrorFunc(virErrorPtr err)
lvl = _("warning");
break;
case VIR_ERR_ERROR:
+ default:
lvl = _("error");
break;
}
@@ -1459,6 +1460,9 @@ virErrorMsg(virErrorNumber error, const char *info)
else
errmsg = _("device not found: %s");
break;
+ default:
+ errmsg = _("unexpected error code");
+ break;
}
return errmsg;
}
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index be40379a92..d877e874e8 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -204,6 +204,7 @@ virFDStreamMsgFree(virFDStreamMsgPtr msg)
VIR_FREE(msg->stream.data.buf);
break;
case VIR_FDSTREAM_MSG_TYPE_HOLE:
+ default:
/* nada */
break;
}
@@ -560,6 +561,9 @@ virFDStreamThreadDoWrite(virFDStreamDataPtr fdst,
pop = true;
break;
+
+ default:
+ break;
}
if (pop) {
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index e7da482640..b50890cef6 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -378,6 +378,7 @@ virFirewallAddRuleFullV(virFirewallPtr firewall,
ADD_ARG(rule, "-w");
break;
case VIR_FIREWALL_LAYER_LAST:
+ default:
break;
}
diff --git a/src/util/virhook.c b/src/util/virhook.c
index facd74aeff..cf104d0234 100644
--- a/src/util/virhook.c
+++ b/src/util/virhook.c
@@ -277,12 +277,12 @@ virHookCall(int driver,
break;
case VIR_HOOK_DRIVER_NETWORK:
opstr = virHookNetworkOpTypeToString(op);
- }
- if (opstr == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Hook for %s, failed to find operation #%d"),
- drvstr, op);
- return 1;
+ break;
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Hook for %s, failed to find operation #%d"),
+ drvstr, op);
+ return 1;
}
subopstr = virHookSubopTypeToString(sub_op);
if (subopstr == NULL)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a12224c58f..889391d45b 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -391,7 +391,6 @@ virHostdevNetConfigVirtPortProfile(const char *linkdev, int vf,
case VIR_NETDEV_VPORT_PROFILE_NONE:
case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
case VIR_NETDEV_VPORT_PROFILE_8021QBG:
- case VIR_NETDEV_VPORT_PROFILE_LAST:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("virtualport type %s is "
"currently not supported on interfaces of type "
@@ -409,6 +408,13 @@ virHostdevNetConfigVirtPortProfile(const char *linkdev, int vf,
macaddr, linkdev, vf,
VIR_NETDEV_VPORT_PROFILE_OP_DESTROY);
break;
+
+ case VIR_NETDEV_VPORT_PROFILE_LAST:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected port profile %d"),
+ virtPort->virtPortType);
+ break;
}
return ret;
diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
index 11efe8c502..26576a73cc 100644
--- a/src/util/virhostmem.c
+++ b/src/util/virhostmem.c
@@ -608,6 +608,11 @@ virHostMemGetParameters(virTypedParameterPtr params ATTRIBUTE_UNUSED,
return -1;
break;
+
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected parameter number %zu"), i);
+ return -1;
}
}
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 14b68b8c93..586feac89f 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -369,6 +369,7 @@ virJSONValueFree(virJSONValuePtr value)
break;
case VIR_JSON_TYPE_BOOLEAN:
case VIR_JSON_TYPE_NULL:
+ default:
break;
}
@@ -1439,6 +1440,10 @@ virJSONValueCopy(const virJSONValue *in)
case VIR_JSON_TYPE_NULL:
out = virJSONValueNewNull();
break;
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected JSON type %d"), in->type);
+ break;
}
return out;
diff --git a/src/util/virlog.c b/src/util/virlog.c
index ecbee71cfb..d36aad3de5 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -253,8 +253,9 @@ virLogPriorityString(virLogPriority lvl)
return "warning";
case VIR_LOG_ERROR:
return "error";
+ default:
+ return "unknown";
}
- return "unknown";
}
@@ -1122,8 +1123,9 @@ int virLogPriorityFromSyslog(int priority)
return VIR_LOG_INFO;
case LOG_DEBUG:
return VIR_LOG_DEBUG;
+ default:
+ return VIR_LOG_ERROR;
}
- return VIR_LOG_ERROR;
}
#else /* HAVE_SYSLOG_H */
@@ -1640,6 +1642,7 @@ virLogParseOutput(const char *src)
#endif
break;
case VIR_LOG_TO_OUTPUT_LAST:
+ default:
break;
}
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index b250af9e2c..b185900bd6 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -2802,6 +2802,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast)
/* coverity[dead_error_begin] */
case VIR_MCAST_TYPE_LAST:
+ default:
break;
}
}
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index fb41bf934c..efd505c062 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -694,6 +694,9 @@ virNetDevMacVLanVPortProfileCallback(struct nlmsghdr *hdr,
((struct ifinfomsg *)data)->ifi_flags);
VIR_DEBUG(" ifi_change = 0x%04x",
((struct ifinfomsg *)data)->ifi_change);
+ break;
+ default:
+ break;
}
/* DEBUG end */
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index 4c0a64e439..1e2a293e8a 100644
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -147,7 +147,7 @@ virNetDevVPortProfileCheckComplete(virNetDevVPortProfilePtr virtport,
if (!virtport || virtport->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)
return 0;
- switch (virtport->virtPortType) {
+ switch ((enum virNetDevVPortProfile)virtport->virtPortType) {
case VIR_NETDEV_VPORT_PROFILE_8021QBG:
if (!virtport->managerID_specified) {
missing = "managerid";
@@ -194,6 +194,15 @@ virNetDevVPortProfileCheckComplete(virNetDevVPortProfilePtr virtport,
if (!virtport->interfaceID_specified)
missing = "interfaceid";
break;
+
+ case VIR_NETDEV_VPORT_PROFILE_NONE:
+ break;
+
+ case VIR_NETDEV_VPORT_PROFILE_LAST:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected port profile type %d"), virtport->virtPortType);
+ return -1;
}
if (missing) {
@@ -220,7 +229,7 @@ virNetDevVPortProfileCheckNoExtras(virNetDevVPortProfilePtr virtport)
if (!virtport || virtport->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)
return 0;
- switch (virtport->virtPortType) {
+ switch ((enum virNetDevVPortProfile)virtport->virtPortType) {
case VIR_NETDEV_VPORT_PROFILE_8021QBG:
if (virtport->profileID[0])
extra = "profileid";
@@ -251,6 +260,16 @@ virNetDevVPortProfileCheckNoExtras(virNetDevVPortProfilePtr virtport)
else if (virtport->instanceID_specified)
extra = "instanceid";
break;
+
+ case VIR_NETDEV_VPORT_PROFILE_MIDONET:
+ case VIR_NETDEV_VPORT_PROFILE_NONE:
+ break;
+
+ case VIR_NETDEV_VPORT_PROFILE_LAST:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected port profile type %d"), virtport->virtPortType);
+ return -1;
}
if (extra) {
@@ -1254,10 +1273,10 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname,
if (!virtPort || vmOp == VIR_NETDEV_VPORT_PROFILE_OP_NO_OP)
return 0;
- switch (virtPort->virtPortType) {
+ switch ((enum virNetDevVPortProfile)virtPort->virtPortType) {
case VIR_NETDEV_VPORT_PROFILE_NONE:
+ case VIR_NETDEV_VPORT_PROFILE_MIDONET:
case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
- case VIR_NETDEV_VPORT_PROFILE_LAST:
break;
case VIR_NETDEV_VPORT_PROFILE_8021QBG:
@@ -1281,6 +1300,12 @@ virNetDevVPortProfileAssociate(const char *macvtap_ifname,
}
break;
+
+ case VIR_NETDEV_VPORT_PROFILE_LAST:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected port profile type %d"), virtPort->virtPortType);
+ break;
}
return rc;
@@ -1319,10 +1344,10 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname,
if (!virtPort)
return 0;
- switch (virtPort->virtPortType) {
+ switch ((enum virNetDevVPortProfile)virtPort->virtPortType) {
case VIR_NETDEV_VPORT_PROFILE_NONE:
+ case VIR_NETDEV_VPORT_PROFILE_MIDONET:
case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
- case VIR_NETDEV_VPORT_PROFILE_LAST:
break;
case VIR_NETDEV_VPORT_PROFILE_8021QBG:
@@ -1341,6 +1366,12 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname,
virtPort, NULL,
VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE);
break;
+
+ case VIR_NETDEV_VPORT_PROFILE_LAST:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected port profile type %d"), virtPort->virtPortType);
+ break;
}
return rc;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index bebe301f8d..bf7db7256a 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -152,7 +152,10 @@ virNumaSetupMemoryPolicy(virDomainNumatuneMemMode mode,
break;
case VIR_DOMAIN_NUMATUNE_MEM_LAST:
- break;
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected NUMA tune mode %d"), mode);
+ goto cleanup;
}
ret = 0;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 1fbbbb3a27..a4801c7f04 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1343,6 +1343,7 @@ virProcessSchedTranslatePolicy(virProcessSchedPolicy policy)
return SCHED_RR;
case VIR_PROC_POLICY_LAST:
+ default:
/* nada */
break;
}
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 2e9e65f9ef..b2932255b2 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -207,6 +207,11 @@ virQEMUBuildCommandLineJSONRecurse(const char *key,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("NULL JSON type can't be converted to commandline"));
return -1;
+
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected JSON type %d"), value->type);
+ return -1;
}
return 0;
diff --git a/src/util/virsexpr.c b/src/util/virsexpr.c
index 885c382a5f..9ed371799e 100644
--- a/src/util/virsexpr.c
+++ b/src/util/virsexpr.c
@@ -78,6 +78,8 @@ sexpr_free(struct sexpr *sexpr)
break;
case SEXPR_NIL:
break;
+ default:
+ break;
}
VIR_FREE(sexpr);
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 95b5274368..078f99775b 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -281,8 +281,9 @@ virSocketAddrEqual(const virSocketAddr *s1, const virSocketAddr *s2)
&s2->data.inet6.sin6_addr.s6_addr,
sizeof(s1->data.inet6.sin6_addr.s6_addr)) == 0 &&
s1->data.inet6.sin6_port == s2->data.inet6.sin6_port);
+ default:
+ return false;
}
- return false;
}
/*
@@ -314,8 +315,9 @@ virSocketAddrIsPrivate(const virSocketAddr *addr)
return ((addr->data.inet6.sin6_addr.s6_addr[0] & 0xFE) == 0xFC ||
((addr->data.inet6.sin6_addr.s6_addr[0] & 0xFF) == 0xFE &&
(addr->data.inet6.sin6_addr.s6_addr[1] & 0xC0) == 0xC0));
+ default:
+ return false;
}
- return false;
}
/*
@@ -334,8 +336,9 @@ virSocketAddrIsWildcard(const virSocketAddr *addr)
sizeof(addr->data.inet4.sin_addr.s_addr)) == 0;
case AF_INET6:
return IN6_IS_ADDR_UNSPECIFIED(&addr->data.inet6.sin6_addr);
+ default:
+ return false;
}
- return false;
}
/*
@@ -1136,9 +1139,9 @@ virSocketAddrIsNumericLocalhost(const char *addr)
sizeof(res.data.inet4.sin_addr.s_addr)) == 0;
case AF_INET6:
return IN6_IS_ADDR_LOOPBACK(&res.data.inet6.sin6_addr);
+ default:
+ return false;
}
-
- return false;
}
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7f878039ba..14d135b389 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -463,6 +463,10 @@ qcow2GetBackingStoreFormat(int *format,
((const char *)buf)+offset);
if (*format <= VIR_STORAGE_FILE_NONE)
return -1;
+ break;
+
+ default:
+ break;
}
offset += len;
@@ -2208,6 +2212,7 @@ virStorageSourceIsLocalStorage(const virStorageSource *src)
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_LAST:
case VIR_STORAGE_TYPE_NONE:
+ default:
return false;
}
@@ -2739,6 +2744,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src,
case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
case VIR_STORAGE_NET_PROTOCOL_LAST:
case VIR_STORAGE_NET_PROTOCOL_NONE:
+ default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("backing store parser is not implemented for protocol %s"),
protocol);
@@ -3491,13 +3497,16 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src,
/* We shouldn't get VOLUME, but the switch requires all cases */
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_NONE:
- case VIR_STORAGE_TYPE_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot retrieve physical for path '%s' type '%s'"),
NULLSTR(src->path),
virStorageTypeToString(actual_type));
return -1;
- break;
+ case VIR_STORAGE_TYPE_LAST:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected storage type %d"), actual_type);
+ return -1;
}
return 0;
@@ -3980,6 +3989,7 @@ virStorageSourceIsRelative(virStorageSourcePtr src)
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_NONE:
case VIR_STORAGE_TYPE_LAST:
+ default:
return false;
}
@@ -4065,6 +4075,7 @@ virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol)
case VIR_STORAGE_NET_PROTOCOL_LAST:
case VIR_STORAGE_NET_PROTOCOL_NONE:
+ default:
return 0;
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
> ---
> src/libvirt.c | 3 +++
> src/util/vircgroup.c | 1 +
> src/util/vircrypto.c | 2 ++
> src/util/virerror.c | 6 +++++-
> src/util/virfdstream.c | 4 ++++
> src/util/virfirewall.c | 1 +
> src/util/virhook.c | 12 +++++------
> src/util/virhostdev.c | 8 +++++++-
> src/util/virhostmem.c | 5 +++++
> src/util/virjson.c | 5 +++++
> src/util/virlog.c | 7 +++++--
> src/util/virnetdev.c | 1 +
> src/util/virnetdevmacvlan.c | 3 +++
> src/util/virnetdevvportprofile.c | 43 ++++++++++++++++++++++++++++++++++------
> src/util/virnuma.c | 5 ++++-
> src/util/virprocess.c | 1 +
> src/util/virqemu.c | 5 +++++
> src/util/virsexpr.c | 2 ++
> src/util/virsocketaddr.c | 13 +++++++-----
> src/util/virstoragefile.c | 15 ++++++++++++--
> 20 files changed, 118 insertions(+), 24 deletions(-)
>
[...]
> diff --git a/src/util/virhook.c b/src/util/virhook.c
> index facd74aeff..cf104d0234 100644
> --- a/src/util/virhook.c
> +++ b/src/util/virhook.c
> @@ -277,12 +277,12 @@ virHookCall(int driver,
Above here at the top of the function there is a :
if ((driver < VIR_HOOK_DRIVER_DAEMON) ||
(driver >= VIR_HOOK_DRIVER_LAST))
return 1;
thus the "default:" case added achieves DEAD_ERROR from Coverity.
> break;
> case VIR_HOOK_DRIVER_NETWORK:
> opstr = virHookNetworkOpTypeToString(op);
> - }
> - if (opstr == NULL) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Hook for %s, failed to find operation #%d"),
> - drvstr, op);
> - return 1;
> + break;
> + default:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Hook for %s, failed to find operation #%d"),
> + drvstr, op);
Removing default: "works" because @driver is an int.
BTW: @drvstr would be NULL here, right?
If the initial condition is removed, then that makes Coverity happy.
> + return 1;
> }
> subopstr = virHookSubopTypeToString(sub_op);
> if (subopstr == NULL)
[...]
> diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
> index 11efe8c502..26576a73cc 100644
> --- a/src/util/virhostmem.c
> +++ b/src/util/virhostmem.c
> @@ -608,6 +608,11 @@ virHostMemGetParameters(virTypedParameterPtr params ATTRIBUTE_UNUSED,
> return -1;
>
> break;
> +
> + default:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unexpected parameter number %zu"), i);
> + return -1;
Here we are in a for loop from 0 to NODE_MEMORY_PARAMETERS_NUM (or 8),
so Coverity complains that default is DEAD... This one's a little
trickier because removing default: leaves open the possibility of the
constant changing and someone not adding the next number in the
sequence. The "easier" path is remove default:, the other option to
avoid Coverity notification is adding a "/* coverity[dead_error_begin]
*/" right above default:.
> }
> }
>
[...]
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index b250af9e2c..b185900bd6 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -2802,6 +2802,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast)
>
> /* coverity[dead_error_begin] */
> case VIR_MCAST_TYPE_LAST:
> + default:
Similar to virhostmem, we're in a for loop where ifindex starts at
VIR_MCAST_TYPE_INDEX_TOKEN and must be less than VIR_MCAST_TYPE_LAST, so
we get a DEAD_ERROR for both of these.
Removing the (virMCastType) and leaving as an int removes the error, but
leaves open the possibility of a new type being missed.
Modifying the code to be:
/* coverity[dead_error_condition] */
case VIR_MCAST_TYPE_LAST:
/* coverity[dead_error_begin] */
default:
Fixes things, but IMO is butt-ugly. So maybe we just leave this one as
is and I keep a local filter for it.
> break;
> }
> }
[...]
I think the first one could be easily adjusted - the last two - IDC,
keep them as is and I'll filter them or adjust them later in my Coverity
branch.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 02/19/2018 06:49 PM, John Ferlan wrote:
>
>
> 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@redhat.com>
>> ---
>> src/libvirt.c | 3 +++
>> src/util/vircgroup.c | 1 +
>> src/util/vircrypto.c | 2 ++
>> src/util/virerror.c | 6 +++++-
>> src/util/virfdstream.c | 4 ++++
>> src/util/virfirewall.c | 1 +
>> src/util/virhook.c | 12 +++++------
>> src/util/virhostdev.c | 8 +++++++-
>> src/util/virhostmem.c | 5 +++++
>> src/util/virjson.c | 5 +++++
>> src/util/virlog.c | 7 +++++--
>> src/util/virnetdev.c | 1 +
>> src/util/virnetdevmacvlan.c | 3 +++
>> src/util/virnetdevvportprofile.c | 43 ++++++++++++++++++++++++++++++++++------
>> src/util/virnuma.c | 5 ++++-
>> src/util/virprocess.c | 1 +
>> src/util/virqemu.c | 5 +++++
>> src/util/virsexpr.c | 2 ++
>> src/util/virsocketaddr.c | 13 +++++++-----
>> src/util/virstoragefile.c | 15 ++++++++++++--
>> 20 files changed, 118 insertions(+), 24 deletions(-)
>>
>
> [...]
>
>> diff --git a/src/util/virhook.c b/src/util/virhook.c
>> index facd74aeff..cf104d0234 100644
>> --- a/src/util/virhook.c
>> +++ b/src/util/virhook.c
>> @@ -277,12 +277,12 @@ virHookCall(int driver,
>
> Above here at the top of the function there is a :
>
> if ((driver < VIR_HOOK_DRIVER_DAEMON) ||
> (driver >= VIR_HOOK_DRIVER_LAST))
> return 1;
>
> thus the "default:" case added achieves DEAD_ERROR from Coverity.
>
>> break;
>> case VIR_HOOK_DRIVER_NETWORK:
>> opstr = virHookNetworkOpTypeToString(op);
>> - }
>> - if (opstr == NULL) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Hook for %s, failed to find operation #%d"),
>> - drvstr, op);
>> - return 1;
>> + break;
>> + default:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Hook for %s, failed to find operation #%d"),
>> + drvstr, op);
>
> Removing default: "works" because @driver is an int.
>
> BTW: @drvstr would be NULL here, right?
>
> If the initial condition is removed, then that makes Coverity happy.
>
>
>> + return 1;
>> }
>> subopstr = virHookSubopTypeToString(sub_op);
>> if (subopstr == NULL)
>
> [...]
>
>> diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
>> index 11efe8c502..26576a73cc 100644
>> --- a/src/util/virhostmem.c
>> +++ b/src/util/virhostmem.c
>> @@ -608,6 +608,11 @@ virHostMemGetParameters(virTypedParameterPtr params ATTRIBUTE_UNUSED,
>> return -1;
>>
>> break;
>> +
>> + default:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Unexpected parameter number %zu"), i);
>> + return -1;
Based on even more warnings like this in the subsequent patch, I'm just
going to disable DEADCODE checking 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 ;-)
John
I'll pick up in the morning on the next set of patches...
>
> Here we are in a for loop from 0 to NODE_MEMORY_PARAMETERS_NUM (or 8),
> so Coverity complains that default is DEAD... This one's a little
> trickier because removing default: leaves open the possibility of the
> constant changing and someone not adding the next number in the
> sequence. The "easier" path is remove default:, the other option to
> avoid Coverity notification is adding a "/* coverity[dead_error_begin]
> */" right above default:.
>
>> }
>> }
>>
>
> [...]
>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index b250af9e2c..b185900bd6 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -2802,6 +2802,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast)
>>
>> /* coverity[dead_error_begin] */
>> case VIR_MCAST_TYPE_LAST:
>> + default:
>
> Similar to virhostmem, we're in a for loop where ifindex starts at
> VIR_MCAST_TYPE_INDEX_TOKEN and must be less than VIR_MCAST_TYPE_LAST, so
> we get a DEAD_ERROR for both of these.
>
> Removing the (virMCastType) and leaving as an int removes the error, but
> leaves open the possibility of a new type being missed.
>
> Modifying the code to be:
>
> /* coverity[dead_error_condition] */
> case VIR_MCAST_TYPE_LAST:
> /* coverity[dead_error_begin] */
> default:
>
> Fixes things, but IMO is butt-ugly. So maybe we just leave this one as
> is and I keep a local filter for it.
>
>> break;
>> }
>> }
>
> [...]
>
> I think the first one could be easily adjusted - the last two - IDC,
> keep them as is and I'll filter them or adjust them later in my Coverity
> branch.
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 15, 2018 at 04:43:21PM +0000, 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 s/existance/existence/ Jan >thus all switches should have a default: case. > >Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.