[libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev

[Erik Skultety]

On Thu, May 31, 2018 at 10:16:41AM +0200, Michal Privoznik wrote:
> On 05/31/2018 10:05 AM, Erik Skultety wrote:
> > There was a missing enum for mdev causing a strange 'unknown device type'
> > warning when hot-plugging mdev.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1583927
> >
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  src/conf/domain_audit.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
> > index 82868bca76..14138d93af 100644
> > --- a/src/conf/domain_audit.c
> > +++ b/src/conf/domain_audit.c
> > @@ -361,6 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev,
> >      virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
> >      virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
> >      virDomainHostdevSubsysSCSIVHostPtr hostsrc = &hostdev->source.subsys.u.scsi_host;
> > +    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &hostdev->source.subsys.u.mdev;
> >
> >      virUUIDFormat(vm->def->uuid, uuidstr);
> >      if (!(vmname = virAuditEncode("vm", vm->def->name))) {
> > @@ -373,9 +374,9 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev,
> >          virt = "?";
> >      }
> >
> > -    switch (hostdev->mode) {
> > +    switch ((virDomainHostdevMode) hostdev->mode) {
> >      case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
> > -        switch (hostdev->source.subsys.type) {
> > +        switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
>
> 1: ^^^
>
> >          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> >              if (virAsprintfQuiet(&address, "%.4x:%.2x:%.2x.%.1x",
> >                                   pcisrc->addr.domain,
> > @@ -419,6 +420,13 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev,
> >                  goto cleanup;
> >              }
> >              break;
> > +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > +            if (VIR_STRDUP_QUIET(address, mdevsrc->uuidstr) < 0) {
> > +                VIR_WARN("OOM while enconding audit message");
> > +                goto cleanup;
> > +            }
> > +            break;
>
> This makes sense.
>
> > +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:>          default:
>
> But this does not. Well, in combination with [1] it doesn't. Firstly,
> why do we even have "default" labels? Secondly, what's the point of

We have them because type casting won't save you from rogue values that can
appear during runtime. Yes, I know, when can that happen if we're in charge,
i.e. it's not a value provided by user? I said something similar at some point
to an email thread on the mailing list that we're in charge here, if we change
an enum value to some garbage we should know about it, ergo the default cases
shouldn't be even needed, but the consensus seemed to be that this kind of
defensive programming doesn't make any performance difference and we are fairly
consistent, we do this on a lot of places, it's a pity I can't find the email
thread to link it here.

> typecasting when we have "default" label? Same goes for the outer

typecasting is for compile-time errors that would tell the programmer about all
the places he missed to add the enum to a switch, which is pretty much the
nature of this bug.

> switch(). I think we should remove "default" labels.

Although you're right because parsing of the XML precedes auditing, having a
default case in a switch doesn't add any overhead, doesn't contribute to the
code being less readable, on the other hand adds some "safety" so I'd like to
keep it in the patch.

Erik