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

[Pavel Hrdina]

On Wed, Dec 09, 2015 at 09:37:53AM +0000, Daniel P. Berrange wrote:
> 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.

Let's hope that they would introduce a way to add them on the command line.

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

In that case, I should rework the patch to use post parse callbacks to add them
to the offline definition.  I didn't think about this scenario.

Thanks,

Pavel

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