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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list