docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 41 ++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 19 ++++++++++ .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++ tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++ 9 files changed, 143 insertions(+) create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
Add SDL graphics gl attribute, modify the domain XML schema, add a
test, modify the documentation to include the new option.
Signed-off-by: Maciej Wolny <maciej.wolny@codethink.co.uk>
---
docs/schemas/domaincommon.rng | 8 +++++
src/conf/domain_conf.c | 41 ++++++++++++++++++++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 19 ++++++++++
.../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++
tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
tests/qemuxml2argvtest.c | 5 +++
9 files changed, 143 insertions(+)
create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
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>
</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;
+ }
+
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) {
+ virBufferAddLit(buf, ">\n");
+ virBufferAdjustIndent(buf, 2);
+ children = true;
+ }
+
+ if (def->data.sdl.gl) {
+ 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,
* default, since the default changes :-( */
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);
+ }
+
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);
DO_TEST("video-virtio-gpu-secondary",
QEMU_CAPS_DEVICE_VIRTIO_GPU,
QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
--
2.11.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 01, 2018 at 08:22:45PM +0100, Maciej Wolny wrote:
> Add SDL graphics gl attribute, modify the domain XML schema, add a
> test, modify the documentation to include the new option.
>
> Signed-off-by: Maciej Wolny <maciej.wolny@codethink.co.uk>
> ---
> docs/schemas/domaincommon.rng | 8 +++++
> src/conf/domain_conf.c | 41 ++++++++++++++++++++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_capabilities.c | 2 ++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 19 ++++++++++
> .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++
> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
> tests/qemuxml2argvtest.c | 5 +++
> 9 files changed, 143 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
>
> 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>
> </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;
> + }
> +
> 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) {
> + virBufferAddLit(buf, ">\n");
> + virBufferAdjustIndent(buf, 2);
> + children = true;
> + }
> +
> + if (def->data.sdl.gl) {
> + virBufferAsprintf(buf, "<gl enable='%s'",
> + virTristateBoolTypeToString(def->data.sdl.gl));
> + virBufferAddLit(buf, "/>\n");
> + }
SPICE GL support allows a "rendernode" property to be set - I'm wondering
if that is relevant to SDL or not ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 02/05/18 08:13, Daniel P. Berrangé wrote:
> On Tue, May 01, 2018 at 08:22:45PM +0100, Maciej Wolny wrote:
>> Add SDL graphics gl attribute, modify the domain XML schema, add a
>> test, modify the documentation to include the new option.
>>
>> Signed-off-by: Maciej Wolny <maciej.wolny@codethink.co.uk>
>> ---
>> docs/schemas/domaincommon.rng | 8 +++++
>> src/conf/domain_conf.c | 41 ++++++++++++++++++++++
>> src/conf/domain_conf.h | 1 +
>> src/qemu/qemu_capabilities.c | 2 ++
>> src/qemu/qemu_capabilities.h | 1 +
>> src/qemu/qemu_command.c | 19 ++++++++++
>> .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++
>> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
>> tests/qemuxml2argvtest.c | 5 +++
>> 9 files changed, 143 insertions(+)
>> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
>> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
>>
>> 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>
>> </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;
>> + }
>> +
>> 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) {
>> + virBufferAddLit(buf, ">\n");
>> + virBufferAdjustIndent(buf, 2);
>> + children = true;
>> + }
>> +
>> + if (def->data.sdl.gl) {
>> + virBufferAsprintf(buf, "<gl enable='%s'",
>> + virTristateBoolTypeToString(def->data.sdl.gl));
>> + virBufferAddLit(buf, "/>\n");
>> + }
>
> SPICE GL support allows a "rendernode" property to be set - I'm wondering
> if that is relevant to SDL or not ?
>
>
> Regards,
> Daniel
>
I purposefully didn't look into this because we didn't need that option
for our use cases - would you still merge this patch without
implementing this option?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 02, 2018 at 11:48:24AM +0100, Maciej Wolny wrote:
> On 02/05/18 08:13, Daniel P. Berrangé wrote:
> > On Tue, May 01, 2018 at 08:22:45PM +0100, Maciej Wolny wrote:
> > > Add SDL graphics gl attribute, modify the domain XML schema, add a
> > > test, modify the documentation to include the new option.
> > >
> > > Signed-off-by: Maciej Wolny <maciej.wolny@codethink.co.uk>
> > > ---
> > > docs/schemas/domaincommon.rng | 8 +++++
> > > src/conf/domain_conf.c | 41 ++++++++++++++++++++++
> > > src/conf/domain_conf.h | 1 +
> > > src/qemu/qemu_capabilities.c | 2 ++
> > > src/qemu/qemu_capabilities.h | 1 +
> > > src/qemu/qemu_command.c | 19 ++++++++++
> > > .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++
> > > tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
> > > tests/qemuxml2argvtest.c | 5 +++
> > > 9 files changed, 143 insertions(+)
> > > create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
> > > create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
> > >
> > > 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>
> > > </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;
> > > + }
> > > +
> > > 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) {
> > > + virBufferAddLit(buf, ">\n");
> > > + virBufferAdjustIndent(buf, 2);
> > > + children = true;
> > > + }
> > > +
> > > + if (def->data.sdl.gl) {
> > > + virBufferAsprintf(buf, "<gl enable='%s'",
> > > + virTristateBoolTypeToString(def->data.sdl.gl));
> > > + virBufferAddLit(buf, "/>\n");
> > > + }
> >
> > SPICE GL support allows a "rendernode" property to be set - I'm wondering
> > if that is relevant to SDL or not ?
> >
> >
> > Regards,
> > Daniel
> >
>
> I purposefully didn't look into this because we didn't need that option for
> our use cases - would you still merge this patch without implementing this
> option?
My thought was that if rendernode is also applicable for SPICE, then
instead of duplicating the struct fields and duplicating parsing, we
should create a helper method for dealing with the <gl> element that
can be shared with SPICE & SDL. If rendernode is not allowed with
SPICE in QEMU, then your simpler approach is sufficient
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 02/05/18 11:54, Daniel P. Berrangé wrote:
> On Wed, May 02, 2018 at 11:48:24AM +0100, Maciej Wolny wrote:
>> On 02/05/18 08:13, Daniel P. Berrangé wrote:
>>> On Tue, May 01, 2018 at 08:22:45PM +0100, Maciej Wolny wrote:
>>>> Add SDL graphics gl attribute, modify the domain XML schema, add a
>>>> test, modify the documentation to include the new option.
>>>>
>>>> Signed-off-by: Maciej Wolny <maciej.wolny@codethink.co.uk>
>>>> ---
>>>> docs/schemas/domaincommon.rng | 8 +++++
>>>> src/conf/domain_conf.c | 41 ++++++++++++++++++++++
>>>> src/conf/domain_conf.h | 1 +
>>>> src/qemu/qemu_capabilities.c | 2 ++
>>>> src/qemu/qemu_capabilities.h | 1 +
>>>> src/qemu/qemu_command.c | 19 ++++++++++
>>>> .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++
>>>> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
>>>> tests/qemuxml2argvtest.c | 5 +++
>>>> 9 files changed, 143 insertions(+)
>>>> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
>>>> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
>>>>
>>>> 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>
>>>> </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;
>>>> + }
>>>> +
>>>> 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) {
>>>> + virBufferAddLit(buf, ">\n");
>>>> + virBufferAdjustIndent(buf, 2);
>>>> + children = true;
>>>> + }
>>>> +
>>>> + if (def->data.sdl.gl) {
>>>> + virBufferAsprintf(buf, "<gl enable='%s'",
>>>> + virTristateBoolTypeToString(def->data.sdl.gl));
>>>> + virBufferAddLit(buf, "/>\n");
>>>> + }
>>>
>>> SPICE GL support allows a "rendernode" property to be set - I'm wondering
>>> if that is relevant to SDL or not ?
>>>
>>>
>>> Regards,
>>> Daniel
>>>
>>
>> I purposefully didn't look into this because we didn't need that option for
>> our use cases - would you still merge this patch without implementing this
>> option?
>
> My thought was that if rendernode is also applicable for SPICE, then
> instead of duplicating the struct fields and duplicating parsing, we
> should create a helper method for dealing with the <gl> element that
> can be shared with SPICE & SDL. If rendernode is not allowed with
> SPICE in QEMU, then your simpler approach is sufficient
>
>
> Regards,
> Daniel
>
Grepping through the source code of QEMU, it seems that the rendernode option
is only used for SPICE (for egl_rendernode_init in ui/spice-core.c).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 08, 2018 at 12:12:42PM +0100, Maciej Wolny wrote:
> On 02/05/18 11:54, Daniel P. Berrangé wrote:
> > On Wed, May 02, 2018 at 11:48:24AM +0100, Maciej Wolny wrote:
> >> On 02/05/18 08:13, Daniel P. Berrangé wrote:
> >>> On Tue, May 01, 2018 at 08:22:45PM +0100, Maciej Wolny wrote:
> >>>> Add SDL graphics gl attribute, modify the domain XML schema, add a
> >>>> test, modify the documentation to include the new option.
> >>>>
> >>>> Signed-off-by: Maciej Wolny <maciej.wolny@codethink.co.uk>
> >>>> ---
> >>>> docs/schemas/domaincommon.rng | 8 +++++
> >>>> src/conf/domain_conf.c | 41 ++++++++++++++++++++++
> >>>> src/conf/domain_conf.h | 1 +
> >>>> src/qemu/qemu_capabilities.c | 2 ++
> >>>> src/qemu/qemu_capabilities.h | 1 +
> >>>> src/qemu/qemu_command.c | 19 ++++++++++
> >>>> .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++
> >>>> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
> >>>> tests/qemuxml2argvtest.c | 5 +++
> >>>> 9 files changed, 143 insertions(+)
> >>>> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
> >>>> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
> >>>>
> >>>> 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>
> >>>> </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;
> >>>> + }
> >>>> +
> >>>> 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) {
> >>>> + virBufferAddLit(buf, ">\n");
> >>>> + virBufferAdjustIndent(buf, 2);
> >>>> + children = true;
> >>>> + }
> >>>> +
> >>>> + if (def->data.sdl.gl) {
> >>>> + virBufferAsprintf(buf, "<gl enable='%s'",
> >>>> + virTristateBoolTypeToString(def->data.sdl.gl));
> >>>> + virBufferAddLit(buf, "/>\n");
> >>>> + }
> >>>
> >>> SPICE GL support allows a "rendernode" property to be set - I'm wondering
> >>> if that is relevant to SDL or not ?
> >>>
> >>>
> >>> Regards,
> >>> Daniel
> >>>
> >>
> >> I purposefully didn't look into this because we didn't need that option for
> >> our use cases - would you still merge this patch without implementing this
> >> option?
> >
> > My thought was that if rendernode is also applicable for SPICE, then
> > instead of duplicating the struct fields and duplicating parsing, we
> > should create a helper method for dealing with the <gl> element that
> > can be shared with SPICE & SDL. If rendernode is not allowed with
> > SPICE in QEMU, then your simpler approach is sufficient
>
> Grepping through the source code of QEMU, it seems that the rendernode option
> is only used for SPICE (for egl_rendernode_init in ui/spice-core.c).
Ok, thanks for checking - sounds like your simple code is fine as is.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/01/2018 03:22 PM, Maciej Wolny wrote:
> Add SDL graphics gl attribute, modify the domain XML schema, add a
> test, modify the documentation to include the new option.
>
> Signed-off-by: Maciej Wolny <maciej.wolny@codethink.co.uk>
> ---
> docs/schemas/domaincommon.rng | 8 +++++
> src/conf/domain_conf.c | 41 ++++++++++++++++++++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_capabilities.c | 2 ++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 19 ++++++++++
> .../qemuxml2argvdata/video-virtio-gpu-sdl-gl.args | 28 +++++++++++++++
> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++++++++++++++++++++
> tests/qemuxml2argvtest.c | 5 +++
> 9 files changed, 143 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args
> create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml
>
My opinion/input - please split into 2 or 3 patches... Makes it easier
to discuss the various components separately. Plus if qemu_capabilities
changes (like it does frequently lately), it's easy to separate out that
patch in order to apply the patches. In more recent times, having the
following seems to work quite well:
patch #1: conf, docs, qemuxml2xmltest, etc. type changes
patch #2: qemu capabilities
patch #3: qemu source, qemuxml2argvtest, etc. type changes
BTW: Since your patches add a capability - I would have expected a
change to add a flag to one (or more) of the
tests/qemucapabilitiesdata/caps_*.xml files, but none are modified. So
that means that the feature may not be introspectable, perhaps it's been
part of qemu since 1.5 or it's something being added to the next qemu
release. If it's a new feature, then when was it added? If it's been
there since 1.5, then no capability flag is required.
How that us determined for sdl is a mystery to me... I usually search
through the qemu */*.json files. In this case I do find some remnants of
'gl' and 'sdl' in qapi/ui.json. But how I read that output is that for
2.12 there's 'sdl' in DisplayOptions, but using 'DisplayNoOpts' which
could mean no options for sdl are allowed, but I'm not sure. Maybe there
is some change in flight - I don't follow qemu-devel that closely.
If I look back at commit id '937ebba00e' for the spice-gl addition
(which yes, was one in one patch) - I can relate that to the similar
spice gl fetch, but looking the recent top of qemu git tree, I don't see
how this same mechanism can be used to determine whether the running
qemu supports sdl using gl.
So for code and other libvirt specific things...
BTW: The docs/formatdomain.html.in to add/describe the 'gl' option for
'sdl' needs to be provided.
> 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.
> </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);
> +
> 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.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 04/05/18 00:08, John Ferlan wrote:
> BTW: Since your patches add a capability - I would have expected a
> change to add a flag to one (or more) of the
> tests/qemucapabilitiesdata/caps_*.xml files, but none are modified. So
> that means that the feature may not be introspectable, perhaps it's been
> part of qemu since 1.5 or it's something being added to the next qemu
> release. If it's a new feature, then when was it added? If it's been
> there since 1.5, then no capability flag is required.
I'm going to need some help understanding the QEMU capabilities test
(tests/qemucapabilitiestest.c). As I understand it, it run QEMU to get
the capabilities and then compares that to the XML file containing the
expected list of capabilities. What are the *.replies files used for?
And how is it do that on multiple architectures and QEMU versions
at the same time?
>
> How that us determined for sdl is a mystery to me... I usually search
> through the qemu */*.json files. In this case I do find some remnants of
> 'gl' and 'sdl' in qapi/ui.json. But how I read that output is that for
> 2.12 there's 'sdl' in DisplayOptions, but using 'DisplayNoOpts' which
> could mean no options for sdl are allowed, but I'm not sure. Maybe there
> is some change in flight - I don't follow qemu-devel that closely.
>
> If I look back at commit id '937ebba00e' for the spice-gl addition
> (which yes, was one in one patch) - I can relate that to the similar
> spice gl fetch, but looking the recent top of qemu git tree, I don't see
> how this same mechanism can be used to determine whether the running
> qemu supports sdl using gl.
It looks like this option was introduced to QEMU in 0b71a5d5. v2.4.0 is
the earliest release which contains that commit.
>
> So for code and other libvirt specific things...
>
> BTW: The docs/formatdomain.html.in to add/describe the 'gl' option for
> 'sdl' needs to be provided.
>
>> 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.
>
>> </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);
>
>
>> +
>> 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.
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/08/2018 10:12 AM, Maciej Wolny wrote: > > > On 04/05/18 00:08, John Ferlan wrote: >> BTW: Since your patches add a capability - I would have expected a >> change to add a flag to one (or more) of the >> tests/qemucapabilitiesdata/caps_*.xml files, but none are modified. So >> that means that the feature may not be introspectable, perhaps it's been >> part of qemu since 1.5 or it's something being added to the next qemu >> release. If it's a new feature, then when was it added? If it's been >> there since 1.5, then no capability flag is required. > > I'm going to need some help understanding the QEMU capabilities test > (tests/qemucapabilitiestest.c). As I understand it, it run QEMU to get > the capabilities and then compares that to the XML file containing the > expected list of capabilities. What are the *.replies files used for? > And how is it do that on multiple architectures and QEMU versions > at the same time? > Sorry for the delay in getting back to you on this... The .replies file is the "response" from qemu for the capabilities requests made against each architecture for each QEMU version they are run against. Let's say you have your qemu 2.12 executable (in my case qemu-system-x86_64), then you run: tests/qemucapsprobe /path/to/qemu-system-x86_64 > tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies And that generates the replies file for at QEMU version and that architecture. That data is sourced by the running the various probes and queries from the virQEMUCaps* structures in qemu_capabilities.c against the version specific emulator file. I see you've posted a v2 - I'll look at that later, but in that posting you only updated the 2.4 caps. I assume you figured out more or less how they work and hand edited the file. While that works, you only modified the 2.4 one and not the caps for 2.5 and beyond... So that probably won't work right, but I'll address that while reviewing. In any case, here's the issue - "I think" - it doesn't seem the -gl option from "sdl" is introspectable. The $QEMU/vl.c code seems to handle the parsing for "sdl" quite differently and that could mean we need to run with the other hack^W mechanism to define a capability... In virQEMUCapsInitQMPMonitor you'll see a series of "if (qemuCaps->version >=" checks which set some specific CAPS flags - I think that's what may need to be done. However, I also note in qemu 2.12, there's a ui.json which seems like it could be a mechanism used to introspect or use QMP in order to determine when/if some command line option exists and has some new flag/option. I'm not sure how the DisplayOptions are used and whether they are even used for sdl -gl. But with the mechanism from the previous paragraph - then at least everything from qemu 2.4 and beyond will get the sdl -gl capability. >> >> How that us determined for sdl is a mystery to me... I usually search >> through the qemu */*.json files. In this case I do find some remnants of >> 'gl' and 'sdl' in qapi/ui.json. But how I read that output is that for >> 2.12 there's 'sdl' in DisplayOptions, but using 'DisplayNoOpts' which >> could mean no options for sdl are allowed, but I'm not sure. Maybe there >> is some change in flight - I don't follow qemu-devel that closely. >> >> If I look back at commit id '937ebba00e' for the spice-gl addition >> (which yes, was one in one patch) - I can relate that to the similar >> spice gl fetch, but looking the recent top of qemu git tree, I don't see >> how this same mechanism can be used to determine whether the running >> qemu supports sdl using gl. > > It looks like this option was introduced to QEMU in 0b71a5d5. v2.4.0 is > the earliest release which contains that commit. > We can use this qemu commit id in the patch 4 of your v2... Thanks for looking it up! John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.