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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
