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

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(a)redhat.com&gt; > > > --- > > > 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.