[libvirt] [PATCH 6/7] device: cleanup input device code

[Daniel P. Berrange]

On Tue, Dec 08, 2015 at 03:02:13PM -0500, Laine Stump wrote:
> On 12/07/2015 06:49 PM, Pavel Hrdina wrote:
> >On Mon, Dec 07, 2015 at 12:36:46PM -0500, Laine Stump wrote:
> >>On 12/04/2015 02:30 PM, Pavel Hrdina wrote:
> >>>The current code was a little bit odd.
> >>Understatement of the Week :-) (also you get bonus points for being polite!)
> >>
> >>>At first we've removed all
> >>>possible implicit input devices from domain definition to add them later
> >>>back if there was any graphics device defined while parsing XML
> >>>description.  That's not all, while formating domain definition to XML
> >>>description we at first ignore any input devices with bus different to
> >>>USB and VIRTIO and few lines later we add implicit input devices to XML.
> >>Looking back at the history, it seems that the bit that ignores
> >>particular input devices when there is a graphics device present was
> >>included in the original commit of domain parsing code; I guess at the
> >>time we didn't think all guest hardware should be represented in the
> >>config. At some later time we decided that "if it's in the guest, it
> >>needs to be in the config", and the 2nd bit that adds in the implicit
> >>devices was added without noticed the earlier bit. Does seem pretty
> >>pointless though :-P
> >>
> >>
> >>>This seems to me as a lot of code for nothing.  This patch could seems
> >>>to be more complicated than original approach, but this is a preferred
> >>>way to modify/add driver specific staff only in those drivers and not
> >>>deal with them in common parsing/formating functions.
> >>>
> >>>The update is to add those implicit input devices only into running XML
> >>>and don't put them automatically to offline XML.  In addition we won't
> >>>remove any input devices specified by user.
> >>I haven't looked at the code yet, but if my understanding of this
> >>description is correct, your changes cause the implicitly added devices
> >>to *not* be stored in the config as written to disk? Sometime a few
> >>years ago, based on problems that users had with OSes complaining of
> >>"hardware changed" I think, we started down the path of "every device
> >>that is in the guest should be represented in the config" in order to
> >>guarantee that those same devices will still be there the next time the
> >>domain is started, even if libvirt changes what it adds implicitly.
> >>
> >>
> >>Or am I jumping to incorrect conclusions about what the patch does?
> >Yes, that's correct, but the downside of this approach is that in the future
> >qemu developers can decide, that some devices are not longer implicit and what
> >if those devices cannot be passed to qemu command line?  This will result in
> >scenario, where you have some implicit devices stored in the XML, but actually
> >the device doesn't exists in the guest.
> 
> Yes, I see your point, and the merit. It does go against what we've
> previously said we want to do though (and implies the need for
> further-reaching changes to be consistent, e.g. the builtin IDE and FDC
> controllers on many architectures including i440fx, and builtin SATA
> controller on Q35). So before we do it, I want to make sure that everyone is
> okay with this.
> 
> Dan?

The scenario Pavel mentions will never happen. If QEMU ever stops implicitly
adding the PS2 mouse+keyboard, it will definitely provide a way to add them
explicitly on the command line. To not do that would be to break the ability
to upgrade to newer QEMU which is not something QEMU would do.

Removing the input devices from the offline XML is really going to be
considered a regression in behaviour. For example, if virt-manager looks
at an inactive guest config, it is going to think there is no mouse or
keyboard available for the guest.

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