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

[Daniel P. Berrangé]

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 at 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 :|