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

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?