3:05 a.m.
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(a)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
3:16 a.m.
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(a)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
3:46 a.m.
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(a)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
3:56 a.m.
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
4:44 a.m.
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
5:47 a.m.
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
2368
days inactive
2368
days old
5 comments
3 participants
participants (3)
-
Erik Skultety -
Michal Privoznik -
Peter Krempa