Re: [libvirt] [PATCH v2 16/42] util: add default: case to all switch statements

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt; John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list