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