[libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
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@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) { 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; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: default: VIR_WARN("Unexpected hostdev type while encoding audit message: %d", hostdev->source.subsys.type); @@ -470,6 +478,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, } break; + case VIR_DOMAIN_HOSTDEV_MODE_LAST: default: VIR_WARN("Unexpected hostdev mode while encoding audit message: %d", hostdev->mode); -- 2.14.3
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@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 typecasting when we have "default" label? Same goes for the outer switch(). I think we should remove "default" labels. Michal
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@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
On Thu, May 31, 2018 at 10:16:41 +0200, Michal Privoznik wrote:
On 05/31/2018 10:05 AM, Erik Skultety wrote:
[...]
@@ -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 typecasting when we have "default" label? Same goes for the outer switch(). I think we should remove "default" labels.
We are doing the opposite now. Some reading you probably missed: https://www.redhat.com/archives/libvir-list/2018-February/msg00728.html
On 05/31/2018 10:56 AM, Peter Krempa wrote:
On Thu, May 31, 2018 at 10:16:41 +0200, Michal Privoznik wrote:
On 05/31/2018 10:05 AM, Erik Skultety wrote:
[...]
@@ -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 typecasting when we have "default" label? Same goes for the outer switch(). I think we should remove "default" labels.
We are doing the opposite now. Some reading you probably missed:
https://www.redhat.com/archives/libvir-list/2018-February/msg00728.html
Ah, good point. ACK and safe for freeze then. Michal
On Thu, May 31, 2018 at 11:44:06AM +0200, Michal Privoznik wrote:
On 05/31/2018 10:56 AM, Peter Krempa wrote:
On Thu, May 31, 2018 at 10:16:41 +0200, Michal Privoznik wrote:
On 05/31/2018 10:05 AM, Erik Skultety wrote:
[...]
@@ -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 typecasting when we have "default" label? Same goes for the outer switch(). I think we should remove "default" labels.
We are doing the opposite now. Some reading you probably missed:
https://www.redhat.com/archives/libvir-list/2018-February/msg00728.html
Ah, good point. ACK and safe for freeze then.
Pushed, thanks. Erik
participants (3)
-
Erik Skultety -
Michal Privoznik -
Peter Krempa