1. Patchew
  2. Libvirt
  3. View series

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

Erik Skultety posted 1 patch 5 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/342b3e48bbc70f9b1bfe508e873a97d003eb0d8b.1527753948.git.eskultet@redhat.com
Test syntax-check passed
src/conf/domain_audit.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
[libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
Posted by Erik Skultety 5 years, 10 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
Posted by Michal Privoznik 5 years, 10 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
Posted by Erik Skultety 5 years, 10 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
Posted by Peter Krempa 5 years, 10 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
Posted by Michal Privoznik 5 years, 10 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
Posted by Erik Skultety 5 years, 10 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.