Re: [libvirt] [Spice-devel] [PATCH v4] qemu: Use heads parameter for QXL driver

On Tue, Jul 21, 2015 at 04:36:45PM +0100, Daniel P. Berrange wrote: >On Tue, Jul 21, 2015 at 11:34:08AM -0400, Laine Stump wrote: >>On 07/21/2015 09:41 AM, Daniel P. Berrange wrote: >>> On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote: >>>> On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote: >>>>> On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote: >>>>>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote: >>>>>>> On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote: >>>>>>>> I spend all morning fixing this to be installed properly in the >>>>>>>> system. Anyway, I finally managed to make this work and found out the >>>>>>>> guest I used for it is not ready to have multiple monitors. Anyway, >>>>>>>> looking at everything else this definitely works, so ACK, I'll push it >>>>>>>> in a while. >>>>>>> I'm still a bit worried that all existing guests will have heads='1' in >>>>>>> their XML as libvirt is adding that by default, but I don't really see a >>>>>>> way around it :-/ We should make sure libvirt stops adding it from now >>>>>>> on though ;) >>>>>>> >>>>>> Well, how would you then limit the outputs to 1? Having said that, I >>>>>> have no idea why we started adding heads="1" automatically and playing >>>>>> with different changes, we have another bug in the parsing/formatting >>>>>> code. You'll also won't be able to migrate from older libvirt with >>>>>> heads='1' to newer ones. I haven't realized that. I'm thinking about >>>>>> reverting the change in libvirt or even using heads > 1. Although >>>>>> that won't migrate either. So the only other thing that we can do is >>>>>> to introduce new parameter, like maxHeads. The sound you just heard >>>>>> is me slapping my face because again there will multiple parameters >>>>>> meaning similar things... >>>>> Introducing a new parameter is not really viable IMHO, because as you >>>>> say it'll leave us with two things meaning the same, and will not be >>>>> compatible with existing apps that know about the current parameter. >>>>> >>>>> I think we need to figure out a way to drop the 'heads=1' from any >>>>> existing configs in /etc/libvirt/qemu when we start up with the new >>>>> libvirtd for the first time. >>>>> >>>> I'm already working on an RFC that would differentiate between loading >>>> XMLs on daemon start-up and defining them. The purpose of that is so >>>> that we can make incompatible XML changes without losing any domains, >>>> but that's not the point here. If we were to drop heads='1' anywhere, >>>> this is the place. The ABI change would be minimal for the guest. >>> It isn't sufficient to just differentiate loading on startup >>> vs defining them IIUC. We need to differentiate loading on >>> startup from XML previously written by a libvirtd < X.Y.Z >>> which is why I think we need to have a version number recorded >>> in the XML ultimately. >> >>But a version number isn't sufficient to describe exactly what the >>previous libvirt was capable of. Many times new features (externally >>visible only in the XML) are backported to earlier versions of libvirt >>downstream (e.g. libvirt-0.10.2 that is used in RHEL6.x and CentOS 6.x), >>so this still doesn't buy us perfection. Downstream maintainers could >>make it better by paying very close attention to any use of this version >>number when backporting new stuff, but there would still be problems if >>someone decided to build and install an upstream libvirt on a system >>that previously had some hybrid downstream build. >> >>(Not saying we shouldn't do it, just that it's no perfect :-) > >Yep, understood. I'm thinking purely in terms of upstream where we do >not backport features to stable branches. Distros which get into the >feature backport game have to deal with that pain themselves. > How about we use some part of the XML to mark "features" there? That part would only be parsed on loading and formatted into migration cookie and the XML saved on disk. We can put it into another namespace. Each feature would have its handler that "fixes" whatever needs to be fixed in the postparse part.