Re: [libvirt] [PATCH v2 3/5] qemu: Add gl property to graphics of type sdl in domain config

On 05/14/2018 09:18 AM, Maciej Wolny wrote: > On 14/05/18 13:40, Martin Kletzander wrote: >> On Mon, May 14, 2018 at 08:27:35AM -0400, John Ferlan wrote: >>> >>> >>> On 05/14/2018 07:24 AM, Martin Kletzander wrote: >>>> On Fri, May 11, 2018 at 03:09:20PM +0100, Maciej Wolny wrote: >>>>> On 11/05/18 09:42, Martin Kletzander wrote: >>>>>> On Thu, May 10, 2018 at 11:53:57AM +0100, Maciej Wolny wrote: >>>>>>> Support OpenGL accelerated rendering when using SDL graphics in the >>>>>>> domain config. Add associated test and documentation. >>>>>>> >>>>>>> Signed-off-by: Maciej Wolny <maciej.wolny(a)codethink.co.uk&gt; >>>>>>> --- >>>>>>> docs/formatdomain.html.in                          |  6 +++ >>>>>>> docs/schemas/domaincommon.rng                      |  8 ++++ >>>>>>> src/conf/domain_conf.c                             | 44 >>>>>>> ++++++++++++++++++++- >>>>>>> src/conf/domain_conf.h                             |  1 + >>>>>> >>>>>> docs, conf and schemas fit together nicely, they should be in one >>>>>> patch, but. >>>>>> >>>>>>> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 >>>>>>> ++++++++++++++++++ >>>>>>> .../qemuxml2xmloutdata/video-virtio-gpu-sdl-gl.xml | 45 >>>>>>> ++++++++++++++++++++++ >>>>>>> tests/qemuxml2xmltest.c                            |  1 + >>>>>> >>>>>> this has nothing to do with qemu (yet), also see Subject (I wouldn't say >>>>>> 'qemu:' there, but rather something like 'docs, conf, schema:') >>>>>> >>>>>> For the XML tests above you can use genericxml2xmltest instead of the >>>>>> QEMU-specific one. >>>>> >>>>> The option only makes sense in QEMU afaik, hence the naming. >>>>> >>>> >>>> Yes, for now.  If someone who's building the code without QEMU driver >>>> changes >>>> the behaviour, the tests will pass if you keep it in qemuxml2xml, however >>>> genericxml2xml will catch that.  qemuxml2xml should be testing specifics >>>> where >>>> you behave based on some more information than just generic XML. >>>> >>>> I hope that's clear. >>>> >>>> Have a nice day. >>>> >>> >>> However, until qemuxml2argvtest can also pull out of genericxml2xmldata, >>> then you'd have separate xml input and output files - is that what's >>> desired? >>> >>> Taking a quick look just now - see the graphics-vnc-socket - do we want >>> to duplicate having two input/output XML files which invariably will >>> diverge? Ironically the generic one has a domain type == qemu, an >>> emulator using qemu, and the socket path using QEMU - so while it's >>> generic in one sense, it's not in others. Even more ironic is the qemu >>> specific file uses "<graphics type='vnc' socket='/tmp/foo.socket'>". >>> >>> Could/should generification of the xml2xml tests be considered a "bite >>> sized task"? >>> >> >> Oh, definitely.  It's only some time ago that the tests started to be usable >> IIRC, so hopefully we'll migrate some XMLs here and there.  But maybe others >> could chime in as well so that I don't speak for others.  I remember Pavel >> having some ideas for cleaner separation of those. > > So, do you guys want to leave that for a separate patch set or do you want me to > post a v3 with the changes Martin has requested? > My opinion is leave it as is - hard to require you to do something we're not requiring other patches at this point. I'm not a fan of duplication - that is the task would be to essentially copy everything into the generic xml2xml test at this point.

John