[libvirt] [PATCH] add fullscreen support to qemu sdl xml (via fullscreen='true' attribute for the graphics element)
diff --git a/src/domain_conf.c b/src/domain_conf.c index 32ed59f..018d07f 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -212,6 +212,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) case VIR_DOMAIN_GRAPHICS_TYPE_SDL: VIR_FREE(def->data.sdl.display); VIR_FREE(def->data.sdl.xauth); + VIR_FREE(def->data.sdl.fullscreen); break; } @@ -1403,6 +1404,7 @@ virDomainGraphicsDefParseXML(virConnectPtr conn, } else if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { def->data.sdl.xauth = virXMLPropString(node, "xauth"); def->data.sdl.display = virXMLPropString(node, "display"); + def->data.sdl.fullscreen = virXMLPropString(node, "fullscreen"); } cleanup: @@ -2951,6 +2953,10 @@ virDomainGraphicsDefFormat(virConnectPtr conn, if (def->data.sdl.xauth) virBufferEscapeString(buf, " xauth='%s'", def->data.sdl.xauth); + if (def->data.sdl.fullscreen) + virBufferEscapeString(buf, " fullscreen='%s'", + def->data.sdl.fullscreen); + break; } diff --git a/src/domain_conf.h b/src/domain_conf.h index 51cf6d5..436d0b1 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -268,6 +268,7 @@ struct _virDomainGraphicsDef { struct { char *display; char *xauth; + char *fullscreen; } sdl; } data; }; diff --git a/src/qemu_conf.c b/src/qemu_conf.c index e6c378f..e53f5f0 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -35,6 +35,7 @@ #include <sys/wait.h> #include <arpa/inet.h> #include <sys/utsname.h> +#include <stdbool.h> #if HAVE_NUMACTL #define NUMA_VERSION1_COMPATIBILITY 1 @@ -1213,6 +1214,7 @@ int qemudBuildCommandLine(virConnectPtr conn, vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { char *xauth = NULL; char *display = NULL; + bool fullscreen = false; if (vm->def->graphics->data.sdl.xauth && asprintf(&xauth, "XAUTHORITY=%s", @@ -1225,10 +1227,17 @@ int qemudBuildCommandLine(virConnectPtr conn, goto no_memory; } + if (vm->def->graphics->data.sdl.fullscreen && + STREQ(vm->def->graphics->data.sdl.fullscreen, "true")) { + fullscreen=true; + } + if (xauth) ADD_ENV(xauth); if (display) ADD_ENV(display); + if (fullscreen) + ADD_ARG_LIT("-full-screen"); } /* Add sound hardware */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl-fullscreen.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl-fullscreen.args new file mode 100644 index 0000000..da2adc0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl-fullscreen.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test XAUTHORITY=/root/.Xauthority DISPLAY=:0.1 /usr/bin/qemu -S -M pc -m 214 -smp 1 -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -full-screen diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl-fullscreen.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl-fullscreen.xml new file mode 100644 index 0000000..007677a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-sdl-fullscreen.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>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</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <input type='mouse' bus='ps2'/> + <graphics type='sdl' display=':0.1' xauth='/root/.Xauthority' fullscreen='true'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6e5355a..a04d508 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -195,6 +195,7 @@ mymain(int argc, char **argv) DO_TEST("disk-usb", 0); DO_TEST("graphics-vnc", 0); DO_TEST("graphics-sdl", 0); + DO_TEST("graphics-sdl-fullscreen", 0); DO_TEST("input-usbmouse", 0); DO_TEST("input-usbtablet", 0); DO_TEST("input-xen", 0); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c810f78..613926a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -98,6 +98,7 @@ mymain(int argc, char **argv) DO_TEST("disk-usb"); DO_TEST("graphics-vnc"); DO_TEST("graphics-sdl"); + DO_TEST("graphics-sdl-fullscreen"); DO_TEST("input-usbmouse"); DO_TEST("input-usbtablet"); DO_TEST("input-xen");
On Tue, Dec 09, 2008 at 07:12:05PM -0500, Itamar Heim wrote:
diff --git a/src/domain_conf.c b/src/domain_conf.c
Hum, I don't know how you sent the diff, but it got broken when reaching my mail agent. Please add as an attachment. Also in general it's a good idea to put some text describing really what the patch does and what is affected, a one line mail subject is a bit short. Especially for thing modifying the XML format, the construct changes ought to be clearly documented. Adding the regression tests is nice, thanks, but we also need to augment the .rng schemas to handle the extension if accepted and the HTML documentation also need the update. We can do it but providing the complete details helps :-) So could you please repost the patch with detail of the syntax change and as an attachment, thanks in advance, Daniel
+ if (vm->def->graphics->data.sdl.fullscreen &&
+ STREQ(vm->def->graphics->data.sdl.fullscreen, "true")) {
+ fullscreen=true;
+ }
+
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Sure - probably not a good idea to send patches at such a late hour anyway :) Attached diff. Some more details: For those using SDL ("for some crazy reason" as Berrange put it in the original patch), there is a use for the fullscreen option, causing the qemu window to open in full screen, change resolution, etc. This patch accepts an additional attribute in the graphics element: <graphics type='sdl' display=':0.1' xauth='/root/.Xauthority' fullscreen='true'/> If fullscreen is true, "-full-screen" is added to the qemu command line (unlike the display and auth which are added as environment variables)
-----Original Message----- From: Daniel Veillard [mailto:veillard@redhat.com] Sent: Wednesday, December 10, 2008 9:49 AM To: Itamar Heim Cc: Libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] add fullscreen support to qemu sdl xml (via fullscreen='true' attribute for the graphics element)
On Tue, Dec 09, 2008 at 07:12:05PM -0500, Itamar Heim wrote:
diff --git a/src/domain_conf.c b/src/domain_conf.c
Hum, I don't know how you sent the diff, but it got broken when reaching my mail agent. Please add as an attachment. Also in general it's a good idea to put some text describing really what the patch does and what is affected, a one line mail subject is a bit short. Especially for thing modifying the XML format, the construct changes ought to be clearly documented. Adding the regression tests is nice, thanks, but we also need to augment the .rng schemas to handle the extension if accepted and the HTML documentation also need the update. We can do it but providing the complete details helps :-)
So could you please repost the patch with detail of the syntax change and as an attachment,
thanks in advance,
Daniel
+ if (vm->def->graphics->data.sdl.fullscreen &&
+ STREQ(vm->def->graphics->data.sdl.fullscreen, "true")) {
+ fullscreen=true;
+ }
+
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Dec 10, 2008 at 06:06:51AM -0500, Itamar Heim wrote:
Sure - probably not a good idea to send patches at such a late hour anyway :) Attached diff.
Some more details: For those using SDL ("for some crazy reason" as Berrange put it in the original patch), there is a use for the fullscreen option, causing the qemu window to open in full screen, change resolution, etc. This patch accepts an additional attribute in the graphics element: <graphics type='sdl' display=':0.1' xauth='/root/.Xauthority' fullscreen='true'/>
If fullscreen is true, "-full-screen" is added to the qemu command line (unlike the display and auth which are added as environment variables)
Looks overall fine to me, the syntactic construct sounds generic enough. I would still suggest 2 more changes: - in virDomainGraphicsDefParseXML check the values coming from the XML, I guess we should accept only "true" and "false" and raise an error, otherwise one may pass --fullscreen=foo on the command line ! - drop stdbool.h and use int instead of bool, this doesn't really gain anything and causes some troubles (e.g. with CIL) Also if you could set your mail agent to set a mime-type of text/plain on the diff or patch attachemnts, that would allow to comment them in-line, which allows for faster feedback loop in the future. thanks :-) ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
-----Original Message----- From: Daniel Veillard [mailto:veillard@redhat.com] Looks overall fine to me, the syntactic construct sounds generic enough. I would still suggest 2 more changes: - in virDomainGraphicsDefParseXML check the values coming from the XML, I guess we should accept only "true" and "false" and raise an error, otherwise one may pass --fullscreen=foo on the command line ! [IH] the qemu command line can only have -full-screen if the attribute is true, no other option, but I can lookup the code checking the input you mentioned. - drop stdbool.h and use int instead of bool, this doesn't really gain anything and causes some troubles (e.g. with CIL) [IH] I was just trying to follow the exiting conventions in the code... see src/cgroup.c Which should it be?
Also if you could set your mail agent to set a mime-type of text/plain on the diff or patch attachemnts, that would allow to comment them in-line, which allows for faster feedback loop in the future.
[IH] will try to remember for patches.
thanks :-) !
Daniel
On Wed, Dec 10, 2008 at 07:16:42AM -0500, Itamar Heim wrote:
-----Original Message----- From: Daniel Veillard [mailto:veillard@redhat.com] Looks overall fine to me, the syntactic construct sounds generic enough. I would still suggest 2 more changes: - in virDomainGraphicsDefParseXML check the values coming from the XML, I guess we should accept only "true" and "false" and raise an error, otherwise one may pass --fullscreen=foo on the command line ! [IH] the qemu command line can only have -full-screen if the attribute is true, no other option, but I can lookup the code checking the input you mentioned.
Oh right, I saw the STREQ but it's better to report the problem when parsing IMHO.
- drop stdbool.h and use int instead of bool, this doesn't really gain anything and causes some troubles (e.g. with CIL)
[IH] I was just trying to follow the exiting conventions in the code... see src/cgroup.c Which should it be?
use int, really...
Also if you could set your mail agent to set a mime-type of text/plain on the diff or patch attachemnts, that would allow to comment them in-line, which allows for faster feedback loop in the future.
[IH] will try to remember for patches.
thanks :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Dec 10, 2008 at 06:06:51AM -0500, Itamar Heim wrote:
Sure - probably not a good idea to send patches at such a late hour anyway :) Attached diff.
In this bit of the code: diff --git a/src/domain_conf.h b/src/domain_conf.h index 51cf6d5..436d0b1 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -268,6 +268,7 @@ struct _virDomainGraphicsDef { struct { char *display; char *xauth; + char *fullscreen; } sdl; } data; }; You shouldn't use a char * for fullscreen. This is a boolean flag, so it should just be an bitfield 'int fullscreen: 1'. This avoids the issue DV pointed out of unexpected values in the XML. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] In this bit of the code:
diff --git a/src/domain_conf.h b/src/domain_conf.h index 51cf6d5..436d0b1 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -268,6 +268,7 @@ struct _virDomainGraphicsDef { struct { char *display; char *xauth; + char *fullscreen; } sdl; } data; };
You shouldn't use a char * for fullscreen. This is a boolean flag, so it should just be an bitfield 'int fullscreen: 1'. This avoids the issue DV pointed out of unexpected values in the XML. [IH] this is used to get the string from the xml, to compare it to the string 'true'. Will the xml parsing infrastructure automatically convert true/false to int and handle parsing errors or I need to write that code in another place?
On Wed, Dec 10, 2008 at 07:49:32AM -0500, Itamar Heim wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] In this bit of the code:
diff --git a/src/domain_conf.h b/src/domain_conf.h index 51cf6d5..436d0b1 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -268,6 +268,7 @@ struct _virDomainGraphicsDef { struct { char *display; char *xauth; + char *fullscreen; } sdl; } data; };
You shouldn't use a char * for fullscreen. This is a boolean flag, so it should just be an bitfield 'int fullscreen: 1'. This avoids the issue DV pointed out of unexpected values in the XML. [IH] this is used to get the string from the xml, to compare it to the string 'true'. Will the xml parsing infrastructure automatically convert true/false to int and handle parsing errors or I need to write that code in another place?
No, do the checking of the XML attribute value at the time it is parsed. For consistency with the VNC display 'autoport' attribute handling, we should actually use 'yes' and 'no' rather than 'true' and 'false'. So, when parsing if ((fullscreen = virXMLPropString(node, "fullscreen")) != NULL) { if (STREQ(fullscreen, "yes")) { def->data.sdl.fullscreen = 1; } VIR_FREE(fullscreen); } And when formatting XML virBufferVSprintf(buf, " fullscreen='%s'", def->data.sdl.fullscreen ? "yes" : "no"); And in the struct int fullscreen : 1; Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Dec 10, 2008 at 06:06:51AM -0500, Itamar Heim wrote:
Sure - probably not a good idea to send patches at such a late hour anyway :) Attached diff.
Some more details: For those using SDL ("for some crazy reason" as Berrange put it in the original patch), there is a use for the fullscreen option, causing the qemu window to open in full screen, change resolution, etc. This patch accepts an additional attribute in the graphics element: <graphics type='sdl' display=':0.1' xauth='/root/.Xauthority' fullscreen='true'/>
If fullscreen is true, "-full-screen" is added to the qemu command line (unlike the display and auth which are added as environment variables)
Okay, to avoid having this patch forgotten in a nitpicking discussion (I'm guilty I started :-) I commited the following which integrate the associated changes and keep the final code in line with the existing one: - make the check at parse time - use an int to store the data - use yes/no as the accepted values - error out if the attribute value does not follow - drop all bool usage - fix docs/libvirt.rng to detail the new optional attribute and its 2 values - fix docs/formatdomain.html[.in] to describe the attributes for the sdl display type. - fix the test to use the new value 'yes' in the xml I guess this covers most of the changes needed for this patch, now you can blame me if I forgot something :-) but this passes the tests (well there is errors on the uml tests I didn't figured out yet but looks unrelated. thanks Itamar ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Thanks! I assume the tests were added as well apart from the patch?
-----Original Message----- From: Daniel Veillard [mailto:veillard@redhat.com] Sent: Thursday, December 11, 2008 13:45 PM To: Itamar Heim Cc: Libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] add fullscreen support to qemu sdl xml (via fullscreen='true' attribute for the graphics element)
On Wed, Dec 10, 2008 at 06:06:51AM -0500, Itamar Heim wrote:
Sure - probably not a good idea to send patches at such a late hour anyway :) Attached diff.
Some more details: For those using SDL ("for some crazy reason" as Berrange put it in the original patch), there is a use for the fullscreen option, causing the qemu window to open in full screen, change resolution, etc. This patch accepts an additional attribute in the graphics element: <graphics type='sdl' display=':0.1' xauth='/root/.Xauthority' fullscreen='true'/>
If fullscreen is true, "-full-screen" is added to the qemu command line (unlike the display and auth which are added as environment variables)
Okay, to avoid having this patch forgotten in a nitpicking discussion (I'm guilty I started :-) I commited the following which integrate the associated changes and keep the final code in line with the existing one: - make the check at parse time - use an int to store the data - use yes/no as the accepted values - error out if the attribute value does not follow - drop all bool usage - fix docs/libvirt.rng to detail the new optional attribute and its 2 values - fix docs/formatdomain.html[.in] to describe the attributes for the sdl display type. - fix the test to use the new value 'yes' in the xml
I guess this covers most of the changes needed for this patch, now you can blame me if I forgot something :-) but this passes the tests (well there is errors on the uml tests I didn't figured out yet but looks unrelated.
thanks Itamar !
Daniel
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Dec 11, 2008 at 06:55:49AM -0500, Itamar Heim wrote:
Thanks! I assume the tests were added as well apart from the patch?
yes even if they don't show in the patch. Just do a cvs update in your checkout (assuming cvs) you should get them ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Dec 11, 2008 at 12:44:32PM +0100, Daniel Veillard wrote:
On Wed, Dec 10, 2008 at 06:06:51AM -0500, Itamar Heim wrote:
Sure - probably not a good idea to send patches at such a late hour anyway :) Attached diff.
Some more details: For those using SDL ("for some crazy reason" as Berrange put it in the original patch), there is a use for the fullscreen option, causing the qemu window to open in full screen, change resolution, etc. This patch accepts an additional attribute in the graphics element: <graphics type='sdl' display=':0.1' xauth='/root/.Xauthority' fullscreen='true'/>
If fullscreen is true, "-full-screen" is added to the qemu command line (unlike the display and auth which are added as environment variables)
Okay, to avoid having this patch forgotten in a nitpicking discussion (I'm guilty I started :-) I commited the following which integrate the associated changes and keep the final code in line with the existing one: - make the check at parse time - use an int to store the data - use yes/no as the accepted values - error out if the attribute value does not follow - drop all bool usage - fix docs/libvirt.rng to detail the new optional attribute and its 2 values - fix docs/formatdomain.html[.in] to describe the attributes for the sdl display type. - fix the test to use the new value 'yes' in the xml
I guess this covers most of the changes needed for this patch, now you can blame me if I forgot something :-) but this passes the tests (well there is errors on the uml tests I didn't figured out yet but looks unrelated.
ACK, the committed patch looks good to me. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Itamar Heim -
Jim Paris