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

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.