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

[Pavel Hrdina]

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?

Definitely we should think about it and take our time to make such decision.

Since you've asked Dan for his opinion, adding him to cc.

> 
> (also, another thing we would want to think about if we do this - do we 
> also want to go through all the existing config files during an upgrade 
> and remove these implicit devices from what's stored on disk? There are 
> pros and cons to both sides of that too)

I think, that we should handle only implicit devices, that we cannot
enable/disable via qemu command line.

> 
> >
> >> I think that whatever hardware is in the guest needs to be represented
> >> in the config, not just in the live XML.
> > I'm not preferring any of those two solutions, but implicit devices that
> > exists in the hypervisor and we cannot do anything about it should be only in
> > live definition.  Than we can simply stop adding those devices into live
> > definition if hypervisor decide to remove those implicit devices instead of
> > removing those devices from persistent definition to make sure, that the
> > definition represents current state of the guest hardware.
> >
> >>> There was also inconsistence between our behavior and QEMU's in the way,
> >>> that in QEMU there is no way how to disable those implicit input devices
> >>> and they are available always, even without graphics device.
> >> Is this the case for every version of qemu that we support?
> > Yes, this applies for every version of qemu so far.
> 
> 
> Okay. I was just curious how that code (that is, making it conditionally 
> happen based on the presence of a video device) got in in the first 
> place - either it was incompletely researched, or qemu has changed.
> 

That was my question too, why this code took assumption, that the input device
is implicitly added only if there is some graphics device.

> 
> >
> >>>     This
> >>> applies also to XEN hypervisor. VZ driver already does its part by
> >>> putting correct implicit devices into live XML.
> >>>
> >>>
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list