On 05/10/2018 05:25 AM, Maciej Wolny wrote:
>
>
> 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.
>
Fair enough - it was more a suggestion. I'll look at v2 in a bit.
>>
>>> </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.
>
I'll look again in v2 - the command line building can be awkward for
those that don't do it often.
>>> +
>>> 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.
Right as I noted in the response to your other posting here - that's
because you only modified the 2.4 capabilities and using the "latest"
probably won't work similarly.
Again I'll look at v2 and we can go from there.
John
> 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
