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

[Erik Skultety]

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 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.

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