On 04/05/18 00:08, John Ferlan wrote:
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 3569b9212..a2ef93c09 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -3031,6 +3031,14 @@
>> <ref name="virYesNo"/>
>> </attribute>
>> </optional>
>> + <optional>
>> + <element name="gl">
>> + <attribute name="enable">
>> + <ref name="virYesNo"/>
>> + </attribute>
>> + <empty/>
>> + </element>
>> + </optional>
>
> I assume this is a copy of the spice gl - to reduce cut-paste-copy, you
> could create a "name" for it and share that name between spice and sdl.
> It's a common thing to do. Not required, but not difficult either.
Unfortunately, the SPICE GL property has a rendernode attribute, while
the SDL one doesn't.. I think this prevents a sensible name definition
which would fit both use cases.
>
>> </group>
>> <group>
>> <attribute name="type">
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b0257068d..7d65ca9df 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13448,6 +13448,7 @@ static int
>> virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
>> xmlNodePtr node)
>> {
>> + xmlNodePtr cur;
>> char *fullscreen = virXMLPropString(node, "fullscreen");
>> int ret = -1;
>>
>> @@ -13468,6 +13469,34 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
>> def->data.sdl.xauth = virXMLPropString(node, "xauth");
>> def->data.sdl.display = virXMLPropString(node, "display");
>>
>> + cur = node->children;
>> + while (cur != NULL) {
>> + if (cur->type == XML_ELEMENT_NODE) {
>> + if (virXMLNodeNameEqual(cur, "gl")) {
>> + char *enable = virXMLPropString(cur, "enable");
>> + int enableVal;
>> +
>> + if (! enable) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("sdl gl element missing enable"));
>> + goto cleanup;
>> + }
>> +
>> + enableVal = virTristateBoolTypeFromString(enable);
>> + if (enableVal < 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown enable value '%s'"), enable);
>> + VIR_FREE(enable);
>> + goto cleanup;
>> + }
>> + VIR_FREE(enable);
>> +
>> + def->data.sdl.gl = enableVal;
>> + }
>> + }
>> + cur = cur->next;
>> + }
>> +
>
> I see this is just a copy of what Spice does, which probably needed some
> adjustment anyway... IIRC: Peter Krempa recently went through an
> exercise with the storage to change a number of these while loops using
> virXMLNodeNameEqual into more direct XML searches. Since this is only
> one element and attribute, I don't really think this loop is useful. I
> know it's possible to get the data via another means and some of the
> code already does that. The exact lines I'll leave to you to hash out.
>
>> ret = 0;
>> cleanup:
>> VIR_FREE(fullscreen);
>> @@ -25652,6 +25681,18 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>> if (def->data.sdl.fullscreen)
>> virBufferAddLit(buf, " fullscreen='yes'");
>>
>> + if (!children && def->data.sdl.gl) {
>
> This should be a comparison such as "def->data.sdl.gl !=
> VIR_TRISTATE_BOOL_ABSENT" (I know it's logically the same, but
> daata.sdl.gl is not a boolean or a pointer).
>
>> + virBufferAddLit(buf, ">\n");
>> + virBufferAdjustIndent(buf, 2);
>> + children = true;
>> + }
>> +
>> + if (def->data.sdl.gl) {
>
> Similarly... yes, I know spice.gl isn't doing that.
>
>> + virBufferAsprintf(buf, "<gl enable='%s'",
>> + virTristateBoolTypeToString(def->data.sdl.gl));
>> + virBufferAddLit(buf, "/>\n");
>> + }
>> +
>> break;
>>
>> case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 3c7eccb8c..90071d9c0 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1601,6 +1601,7 @@ struct _virDomainGraphicsDef {
>> char *display;
>> char *xauth;
>> bool fullscreen;
>> + virTristateBool gl;
>> } sdl;
>> struct {
>> int port;
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index aa8d350f5..02680502e 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -474,6 +474,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>> "query-cpus-fast",
>> "disk-write-cache",
>> "nbd-tls",
>> + "sdl-gl",
>> );
>>
>>
>> @@ -2451,6 +2452,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 },
>> };
>>
>> static int
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index 2afe7ef58..e36611e2a 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -458,6 +458,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
>> QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */
>> QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */
>> QEMU_CAPS_NBD_TLS, /* NBD server supports TLS transport */
>> + QEMU_CAPS_SDL_GL, /* -sdl gl */
>>
>> QEMU_CAPS_LAST /* this must always be the last item */
>> } virQEMUCapsFlags;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 418729b98..29214e806 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7988,6 +7988,7 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
>> virQEMUCapsPtr qemuCaps,
>> virDomainGraphicsDefPtr graphics)
>> {
>> + virBuffer opt = VIR_BUFFER_INITIALIZER;
>> switch (graphics->type) {
>> case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
>> if (graphics->data.sdl.xauth)
>> @@ -8009,6 +8010,24 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
>
> Suggestion... Create a patch prior to this one that moves the code for
> VIR_DOMAIN_GRAPHICS_TYPE_SDL into it's own helper - such as
> qemuBuildGraphicsSDLCommandLine.
>
>> * default, since the default changes :-( */
>
> The above comment can be removed in a separate patch since commit id
> '4e8993a2' removed the check for QEMU_CAPS_SDL, although it took until
> commit id '649a9dd7a' to "fix" the QEMU_CAPS_SDL to be X_QEMU_CAPS_SDL
> since the QEMU 1.5 is our new minimum version supported.
>
>> virCommandAddArg(cmd, "-sdl");
>>
>> + if (graphics->data.sdl.gl == VIR_TRISTATE_BOOL_YES) {
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL_GL)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("This QEMU doesn't support SDL OpenGL"));
>> + return -1;
>> +
>> + }
>> +
>> + virBufferAsprintf(&opt, "gl=%s",
>> + virTristateSwitchTypeToString(graphics->data.sdl.gl));
>> + }
>> +
>> + {
>> + const char *optContent = virBufferCurrentContent(&opt);
>> + if (optContent && STRNEQ(optContent, ""))
>> + virCommandAddArgBuffer(cmd, &opt);
>> + }
>
> With it's own helper ^^this^^ awkward definition in the middle is
> unnecessary I think... In fact, I don't even know why it matters - I
> would think you could just do the virCommandAddArgBuffer right after the
> virBufferAsprintf for "gl=%s"... I think the separate helper will be
> good though in case there's more things added for sdl.
>
> e.g.:
> virCommandAddArgBuffer(cmd, &opt);
> virBufferFreeAndReset(&opt);
>
>
Without that 'if', it'll append an empty, quoted string ("''") to the
command line. The lexical scope in there was indeed unnecessary.
>> +
>> break;
>>
>> case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
>> diff --git a/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
>> new file mode 100644
>> index 000000000..4172320ed
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
>> @@ -0,0 +1,28 @@
>> +LC_ALL=C \
>> +PATH=/bin \
>> +HOME=/home/test \
>> +USER=test \
>> +LOGNAME=test \
>> +/usr/bin/qemu-system-i686 \
>> +-name QEMUGuest1 \
>> +-S \
>> +-machine pc,accel=tcg,usb=off,dump-guest-core=off \
>> +-m 1024 \
>> +-smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>> +-no-user-config \
>> +-nodefaults \
>> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
>> +server,nowait \
>> +-mon chardev=charmonitor,id=monitor,mode=control \
>> +-rtc base=utc \
>> +-no-shutdown \
>> +-no-acpi \
>> +-boot c \
>> +-usb \
>> +-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\
>> +id=drive-ide0-0-0,cache=none \
>> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
>> +-sdl gl=on \
>> +-device virtio-gpu-pci,id=video0,virgl=on,bus=pci.0,addr=0x2 \
>> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>> diff --git a/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
>> new file mode 100644
>> index 000000000..9dea73fbe
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
>> @@ -0,0 +1,38 @@
>> +<domain type='qemu'>
>> + <name>QEMUGuest1</name>
>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> + <memory unit='KiB'>1048576</memory>
>> + <currentMemory unit='KiB'>1048576</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='i686' machine='pc'>hvm</type>
>> + <boot dev='hd'/>
>> + </os>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-i686</emulator>
>> + <disk type='file' device='disk'>
>> + <driver name='qemu' type='qcow2' cache='none'/>
>> + <source file='/var/lib/libvirt/images/QEMUGuest1'/>
>> + <target dev='hda' bus='ide'/>
>> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>> + </disk>
>> + <controller type='ide' index='0'/>
>> + <controller type='usb' index='0'/>
>> + <controller type='pci' index='0' model='pci-root'/>
>> + <input type='mouse' bus='ps2'/>
>> + <input type='keyboard' bus='ps2'/>
>> + <graphics type='sdl'>
>> + <gl enable='yes'/>
>> + </graphics>
>> + <video>
>> + <model type='virtio' heads='1'>
>> + <acceleration accel3d='yes'/>
>> + </model>
>> + </video>
>> + <memballoon model='virtio'/>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 5b3bd4a99..0b06699f0 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -1924,6 +1924,11 @@ mymain(void)
>> QEMU_CAPS_SPICE_GL,
>> QEMU_CAPS_SPICE_RENDERNODE,
>> QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
>> + DO_TEST("video-virtio-gpu-sdl-gl",
>> + QEMU_CAPS_DEVICE_VIRTIO_GPU,
>> + QEMU_CAPS_VIRTIO_GPU_VIRGL,
>> + QEMU_CAPS_SDL_GL,
>> + QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
>
> Newer code should use DO_TEST_CAPS_LATEST, but I think that'll fail
> based on there being no capability defined in latest.
I haven't managed to make that working with DO_TEST_CAPS_LATEST.
I need more information on how QEMU capabilities work.. Am I right to
think that the actual capabilities will depend not only on the version
of QEMU but also other packages in the system (say, virgl will require
libvirglrenderer)? If so, do the files in tests/qemucapabilitiesdata
only list basic capabilities that are always provided by that version of
QEMU? Adding QEMU_CAPS_SDL_GL to the latest caps *.xml file isn't enough,
that test will also need virgl (which is not listed in that file).
>
> The only reason we're printing the "-sdl gl=on" in the .args file is
> that you've passed the capability here.
>
>> DO_TEST("video-virtio-gpu-secondary",
>> QEMU_CAPS_DEVICE_VIRTIO_GPU,
>> QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
>>
>
> There needs to be an adjustment to qemuxml2xmltest.c as well. That'll
> cause an output file to be generated/compared against as well. Hint,
> use VIR_TEST_REGENERATE_OUTPUT=1 after touching the output file.
>
> John
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
