1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/4] Introduce support for mediated devices hot plug
  4. View patch

[libvirt] [PATCH 1/4] qemu: hotplug: Provide a string of a subsystem type instead of an int

Erik Skultety posted 4 patches 7 years, 1 month ago
[libvirt] [PATCH 1/4] qemu: hotplug: Provide a string of a subsystem type instead of an int
Posted by Erik Skultety 7 years, 1 month ago 
If one tries to detach a non-existent device, the error they get is:
"Unexpected hostdev type <int>". Let's use ToString conversion, since
the XML parser would have complained already if the type to be unplugged
was unknown to libvirt.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/qemu/qemu_hotplug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 49af4d4ff..6ec401e21 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
             break;
         default:
             virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unexpected hostdev type %d"), subsys->type);
+                           _("unexpected hostdev type '%s'"),
+                           virDomainHostdevSubsysTypeToString(subsys->type));
             break;
         }
         return -1;
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: hotplug: Provide a string of a subsystem type instead of an int
Posted by Peter Krempa 7 years, 1 month ago 
On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
> If one tries to detach a non-existent device, the error they get is:
> "Unexpected hostdev type <int>". Let's use ToString conversion, since
> the XML parser would have complained already if the type to be unplugged
> was unknown to libvirt.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 49af4d4ff..6ec401e21 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
>              break;
>          default:
>              virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("unexpected hostdev type %d"), subsys->type);
> +                           _("unexpected hostdev type '%s'"),
> +                           virDomainHostdevSubsysTypeToString(subsys->type));

If the type is out of range of the enum this will return NULL. So you
need to use NULLSTR()

ACK with that tweak
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: hotplug: Provide a string of a subsystem type instead of an int
Posted by Daniel P. Berrangé 7 years, 1 month ago 
On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
> On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
> > If one tries to detach a non-existent device, the error they get is:
> > "Unexpected hostdev type <int>". Let's use ToString conversion, since
> > the XML parser would have complained already if the type to be unplugged
> > was unknown to libvirt.
> > 
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/qemu/qemu_hotplug.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 49af4d4ff..6ec401e21 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> >              break;
> >          default:
> >              virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                           _("unexpected hostdev type %d"), subsys->type);
> > +                           _("unexpected hostdev type '%s'"),
> > +                           virDomainHostdevSubsysTypeToString(subsys->type));
> 
> If the type is out of range of the enum this will return NULL. So you
> need to use NULLSTR()
> 
> ACK with that tweak

Actually any default: or _LAST: case should use virReportEnumRangeError().
The virDomainHostdevSubsysTypeToString() methods should only be used in
explicitly matched enums constants.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: hotplug: Provide a string of a subsystem type instead of an int
Posted by Erik Skultety 7 years, 1 month ago 
On Tue, Mar 27, 2018 at 10:24:54AM +0100, Daniel P. Berrangé wrote:
> On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
> > On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
> > > If one tries to detach a non-existent device, the error they get is:
> > > "Unexpected hostdev type <int>". Let's use ToString conversion, since
> > > the XML parser would have complained already if the type to be unplugged
> > > was unknown to libvirt.
> > >
> > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > ---
> > >  src/qemu/qemu_hotplug.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > > index 49af4d4ff..6ec401e21 100644
> > > --- a/src/qemu/qemu_hotplug.c
> > > +++ b/src/qemu/qemu_hotplug.c
> > > @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> > >              break;
> > >          default:
> > >              virReportError(VIR_ERR_INTERNAL_ERROR,
> > > -                           _("unexpected hostdev type %d"), subsys->type);
> > > +                           _("unexpected hostdev type '%s'"),
> > > +                           virDomainHostdevSubsysTypeToString(subsys->type));
> >
> > If the type is out of range of the enum this will return NULL. So you
> > need to use NULLSTR()
> >
> > ACK with that tweak
>
> Actually any default: or _LAST: case should use virReportEnumRangeError().
> The virDomainHostdevSubsysTypeToString() methods should only be used in
> explicitly matched enums constants.

I understand having this kind of safe guard, but we have a specific code path
here where the XML parser has already taken care of non-existent types and
we're left with only types which do not support or do not care about hotplug,
therefore an error like "Unexpected value <number> blah" doesn't IMHO make
sense and doesn't tell you anything, we should reformulate the whole
'unexpected' part of the error, we expected it just fine, it's just there's no
such device in the domain and even if it was, we don't know whether we can do
hotplug on it, that's what you can read from the code as you follow the detach
procedure.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: hotplug: Provide a string of a subsystem type instead of an int
Posted by Daniel P. Berrangé 7 years, 1 month ago 
On Tue, Mar 27, 2018 at 03:42:08PM +0200, Erik Skultety wrote:
> On Tue, Mar 27, 2018 at 10:24:54AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
> > > On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
> > > > If one tries to detach a non-existent device, the error they get is:
> > > > "Unexpected hostdev type <int>". Let's use ToString conversion, since
> > > > the XML parser would have complained already if the type to be unplugged
> > > > was unknown to libvirt.
> > > >
> > > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > > ---
> > > >  src/qemu/qemu_hotplug.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > > > index 49af4d4ff..6ec401e21 100644
> > > > --- a/src/qemu/qemu_hotplug.c
> > > > +++ b/src/qemu/qemu_hotplug.c
> > > > @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> > > >              break;
> > > >          default:
> > > >              virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > -                           _("unexpected hostdev type %d"), subsys->type);
> > > > +                           _("unexpected hostdev type '%s'"),
> > > > +                           virDomainHostdevSubsysTypeToString(subsys->type));
> > >
> > > If the type is out of range of the enum this will return NULL. So you
> > > need to use NULLSTR()
> > >
> > > ACK with that tweak
> >
> > Actually any default: or _LAST: case should use virReportEnumRangeError().
> > The virDomainHostdevSubsysTypeToString() methods should only be used in
> > explicitly matched enums constants.
> 
> I understand having this kind of safe guard, but we have a specific code path
> here where the XML parser has already taken care of non-existent types and
> we're left with only types which do not support or do not care about hotplug,
> therefore an error like "Unexpected value <number> blah" doesn't IMHO make
> sense and doesn't tell you anything, we should reformulate the whole
> 'unexpected' part of the error, we expected it just fine, it's just there's no
> such device in the domain and even if it was, we don't know whether we can do
> hotplug on it, that's what you can read from the code as you follow the detach
> procedure.

The point is to be robust against mistakes in the XML parser, or code that
runs after it. We've had cases in the code where we parsed the wrong enum
into a field, so we would have ended up with unexpected values. Or something
could mistakenly overwrite the type value after parsing but before calling
this function.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: hotplug: Provide a string of a subsystem type instead of an int
Posted by Erik Skultety 7 years, 1 month ago 
On Tue, Mar 27, 2018 at 02:46:49PM +0100, Daniel P. Berrangé wrote:
> On Tue, Mar 27, 2018 at 03:42:08PM +0200, Erik Skultety wrote:
> > On Tue, Mar 27, 2018 at 10:24:54AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
> > > > On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
> > > > > If one tries to detach a non-existent device, the error they get is:
> > > > > "Unexpected hostdev type <int>". Let's use ToString conversion, since
> > > > > the XML parser would have complained already if the type to be unplugged
> > > > > was unknown to libvirt.
> > > > >
> > > > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > > > ---
> > > > >  src/qemu/qemu_hotplug.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > > > > index 49af4d4ff..6ec401e21 100644
> > > > > --- a/src/qemu/qemu_hotplug.c
> > > > > +++ b/src/qemu/qemu_hotplug.c
> > > > > @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> > > > >              break;
> > > > >          default:
> > > > >              virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > > -                           _("unexpected hostdev type %d"), subsys->type);
> > > > > +                           _("unexpected hostdev type '%s'"),
> > > > > +                           virDomainHostdevSubsysTypeToString(subsys->type));
> > > >
> > > > If the type is out of range of the enum this will return NULL. So you
> > > > need to use NULLSTR()
> > > >
> > > > ACK with that tweak
> > >
> > > Actually any default: or _LAST: case should use virReportEnumRangeError().
> > > The virDomainHostdevSubsysTypeToString() methods should only be used in
> > > explicitly matched enums constants.
> >
> > I understand having this kind of safe guard, but we have a specific code path
> > here where the XML parser has already taken care of non-existent types and
> > we're left with only types which do not support or do not care about hotplug,
> > therefore an error like "Unexpected value <number> blah" doesn't IMHO make
> > sense and doesn't tell you anything, we should reformulate the whole
> > 'unexpected' part of the error, we expected it just fine, it's just there's no
> > such device in the domain and even if it was, we don't know whether we can do
> > hotplug on it, that's what you can read from the code as you follow the detach
> > procedure.
>
> The point is to be robust against mistakes in the XML parser, or code that
> runs after it. We've had cases in the code where we parsed the wrong enum
> into a field, so we would have ended up with unexpected values. Or something
> could mistakenly overwrite the type value after parsing but before calling
> this function.

Fair enough, I'm going to drop this patch for the time being and create a
bite-sized task to add virReportEnumRangeError to all the places where it's
missing.

Erik

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.