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