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é <berra...@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 <jfer...@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
