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

[John Ferlan]

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 at 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 at redhat.com>

John