Re: [libvirt] [PATCH v2 4/5] qemu: Add QEMU_CAPS_SDL_GL to qemu capabilities

On Fri, May 11, 2018 at 06:34:54AM -0400, John Ferlan wrote: > On 05/11/2018 04:26 AM, Martin Kletzander wrote: >> On Thu, May 10, 2018 at 04:17:52PM -0400, John Ferlan wrote: >>> >>> >>> On 05/10/2018 06:53 AM, Maciej Wolny wrote: >>>> Support OpenGL acceleration capability when using SDL graphics. >>>> >>>> Signed-off-by: Maciej Wolny <maciej.wolny(a)codethink.co.uk&gt; >>>> --- >>>>  src/qemu/qemu_capabilities.c                         | 2 ++ >>>>  src/qemu/qemu_capabilities.h                         | 1 + >>>>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 9 +++++++++ >>>>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml     | 3 ++- >>>>  4 files changed, 14 insertions(+), 1 deletion(-) >>>> >>> >>> As I rather lengthily noted in the v1 - I'm assuming you handed >>> edited the .replies file.  What that perhaps works and gets you >>> the answer, the fact that none of other files were adjusted leads >>> me to the hand editing belief. >>> >>>> diff --git a/src/qemu/qemu_capabilities.c >>>> b/src/qemu/qemu_capabilities.c >>>> index 920a59617..23f917b66 100644 >>>> --- a/src/qemu/qemu_capabilities.c >>>> +++ b/src/qemu/qemu_capabilities.c >>>> @@ -475,6 +475,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, >>>>                "disk-write-cache", >>>>                "nbd-tls", >>>>                "tpm-crb", >>>> +              "sdl-gl", >>>>      ); >>>> >>>> >>>> @@ -2456,6 +2457,7 @@ static struct virQEMUCapsCommandLineProps >>>> virQEMUCapsCommandLine[] = { >>>>      { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, >>>>      { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, >>>>      { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, >>>> +    { "sdl", "gl", QEMU_CAPS_SDL_GL }, >>>>  }; >>> >>> Rather than this, I'll apply the following diff: >>> >>> diff --git a/src/qemu/qemu_capabilities.c >>> b/src/qemu/qemu_capabilities.c >>> index 23f917b66e..28079fa7ab 100644 >>> --- a/src/qemu/qemu_capabilities.c >>> +++ b/src/qemu/qemu_capabilities.c >>> @@ -2457,7 +2457,6 @@ static struct virQEMUCapsCommandLineProps >>> virQEMUCapsCommandLine[] = { >>>     { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, >>>     { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, >>>     { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, >>> -    { "sdl", "gl", QEMU_CAPS_SDL_GL }, >>> }; >>> >>> static int >>> @@ -3828,6 +3827,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr >>> qemuCaps, >>>     if (qemuCaps->version >= 2004000) >>>         virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_SMM_OPT); >>> >>> +    /* sdl -gl option is supported from v2.4.0 (qemu commit id >>> 0b71a5d5) */ >>> +    if (qemuCaps->version >= 2004000) >>> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_SDL_GL); >>> + >>>     /* Since 2.4.50 ARM virt machine supports gic-version option */ >>>     if (qemuCaps->version >= 2004050) >>>         virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACH_VIRT_GIC_VERSION); >>> >>> >> >> NO! Why? We only result to setting capabilities by version if there is >> no other >> way to do that.  If query-commandline-options (or whatever that name is) >> works >> for this capability, why would you even consider this change? >> >> NACK to the diff you added. > > Are you sure? Did you read my responses from v1: > > https://www.redhat.com/archives/libvir-list/2018-May/msg00250.html > https://www.redhat.com/archives/libvir-list/2018-May/msg00671.html > > I actually spent some time trying to figure out which magic incantation > of the virQEMUCaps* would work. I even tried various forms in > virQEMUCapsQMPSchemaQueries, but could not get the flag to be added from > qemu 2.4 and beyond.  Again, my "assumption" is that it was hand edited > into the 2.4 replies that was added here mainly because none of the > other available sdl * options were addressed - just the one that was > wanted. > > Looking at the QEMU source code - AFAICT the gl option is only "known" > or handled within parse_options of vl.c.  I found nothing in the .json > files where I'd usually find some mechanism that would allow > introspection for the "-sdl gl" option. There is something in > qapi/ui.json that has 'gl' that looks like it could be used in some new > command syntax, but that seems to imply QEMU 2.12 only. > > Please enlighten me/us with code that will work before just pushing the > NACK button. > I'm so sorry for that.  I totally missed the part that the reply was hand-edited.  In that case, unfortunately, the only way is to use the version _again_.  So my apologies once again, especially if that sounded rude (which it does to me now when I'm reading after myself).  I am adding a capability currently and I'm dealing with part of code that I reworked few times due to a similar misunderstanding from a previous developer.

So feel free to squash that in if Maciej is OK with it as well (which he should be, otherwise it won't work for anyone anyway).  ACK. > Tks, > > John