[libvirt] [RFC] Can we error out early for unknown device models?

[Daniel P. Berrange]

On Mon, Oct 27, 2014 at 01:42:59PM +0100, Martin Kletzander wrote:
> On Fri, Oct 24, 2014 at 11:17:48AM +0100, Daniel P. Berrange wrote:
> >On Fri, Oct 24, 2014 at 12:15:24PM +0200, Martin Kletzander wrote:
> >>On Fri, Oct 24, 2014 at 07:16:44AM +0200, Michal Privoznik wrote:
> >>>On 22.10.2014 13:58, Martin Kletzander wrote:
> >>>>Hi everyone,
> >>>>
> >>>>I had this idea that since we are probing QEMU binaries for devices
> >>>>using 'qom-list-types', we could store that data in the capabilities
> >>>>and check whether device models are supported before starting QEMU.
> >>>>We do that for _some newer_ devices, but this would be global.  It
> >>>>would help out particularly with devices like the following one, for
> >>>>example:
> >>>>
> >>>><interface type='network'>
> >>>>   <source network='default'/>
> >>>>   <model type='non-existing_device'/>
> >>>></interface>
> >>>>
> >>>>where we simply construct QEMU command-line with that device model and
> >>>>it then properly errors out:
> >>>>
> >>>>error: internal error: process exited while connecting to monitor:
> >>>>qemu-system-x86_64: -device non-existing_device,...: Parameter
> >>>>'driver' expects device type
> >>>>
> >>>>This would be easy to achieve with current data unless there are some
> >>>>models hidden from qom-list-types' output.  I hope there are none.
> >>>
> >>>Well, this may vary among distros. While one distro lives with bleeding
> >>>edge, the other may just backport some patches. Even those adding new
> >>>devices. So I wouldn't rely on that.
> >>>
> >>
> >>I don't quite get what you mean.  We'd be still compatible with old
> >>QEMU binaries that don't provide the data.
> >>
> >>>>
> >>>>When I checked the output of 'qemu-kvm -device \?', the devices listed
> >>>>there are separated into categories and have buses assigned to them
> >>>>and that lead me to another idea.  What if that data is added to
> >>>>qom-list-types' output and used in libvirt as well?  We could then
> >>>>solve another annoying cases like misusing devices or plugging them
> >>>>into unsupported buses.  Although I don't know any person who would do
> >>>>such a thing, even when solving the first idea, there'd still be a
> >>>>possibility to do a thing like:
> >>>>
> >>>><interface type='network'>
> >>>>   <source network='default'/>
> >>>>   <model type='AC97'/>
> >>>></interface>
> >>>>
> >>>>This idea is obviously meant for the QEMU driver, but if there's
> >>>>something similar in other drivers, it might be useful as well.
> >>>>
> >>>>I'd be interested in any feedback and welcome any ideas to whether it
> >>>>is useful, how the storing should be done or even if it's something we
> >>>>want to have or not.
> >>>
> >>>So do you suggest that the check should be done on domain startup or
> >>>domain define time? It can't be the latter - libvirt should expose all
> >>>the domains it knows of - even these that can't be started. The idea
> >>>was, users can downgrade qemu and libvirt should cope with that.
> >>>If you, however, intend to do this at runtime, I see this as a gap in
> >>>our capabilites querying/checking. Or am I missing something here?
> >>>
> >>
> >>Wherever the capabilities are checked now.  Of course you cannot do
> >>this when defining the domain.  This would be just an additional check
> >>when building the command line.  And the gap is what I'm suggesting to
> >>fix.  Just to be clear, the only output of such patches would be a
> >>nicer error message without relying on starting QEMU, nothing more.
> >
> >If we can get better error reporting of devices that QEMU does not
> >support, when we start QEMU that is a win for me.  QEMU has a really
> >awful error message for devices that don't exist.
> >
> 
> Yes, because it thinks the device name is a parameter because no such
> device exists or something like that.  Thanks for that, I'll
> definitely try to come up with a unified solution.
> 
> However, my question extends to the second use case, which is even
> more corner-case (I really don't think anyone is going to try that and
> bug us about it), but I want to make sure, does it make sense to
> extend qemu's qom-list-types with device category (and supported
> buses) only so we can error out with:
> 
>  Not supported: Cannot use AC97 as an interface model.
> 
> instead of:
> 
>  error: internal error: process exited while connecting to monitor:
>  qemu-system-x86_64: -device AC97,...: Property '.netdev' not found
> 
> I, personally, think it is a bit of an overkill.  I just want to have
> more general opinion.

As  a general rule we whitelist the models we allow through from the
XML parsing. IIRC, the <interface> model is the only place we don't
explicitly whitelist them.  If we fix that, then it will be impossible
for libvirt to generate a command line where we pass a NIC model where
QEMU wants a disk model or vica-verca. So I don't think we need todo
any further validation in this respect

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|