[Libvirt-cim] [PATCH] Add SDL graphic device support

Fri Jul 15 15:14:13 UTC 2011

On 07/14/2011 03:32 AM, Wayne Xia wrote:
> # HG changeset patch
> # User Wayne Xia <xiawenc at linux.vnet.ibm.com>
> # Date 1310271518 -28800
> # Node ID ec5e5be04391afa9bab3b4a59bc5596a68a9f175
> # Parent 395f2d684c1046455462db7e4e87d30e7aae0feb
> Add SDL graphic device support
>
> the options are described in ResourceAllocationSettingData.mof
>

Thanks for the patch!

Please provide the description also in the commit message. It is also 
important to explain how your patch works and maybe the reasons of why 
you decided to do things this way. See below.

> Signed-off-by: Wayne Xia <xiawenc at linux.vnet.ibm.com>
>
> diff -r 395f2d684c10 -r ec5e5be04391 libxkutil/device_parsing.c
> --- a/libxkutil/device_parsing.c Tue Jul 05 15:52:31 2011 -0300
> +++ b/libxkutil/device_parsing.c Sun Jul 10 12:18:38 2011 +0800
> @@ -90,11 +90,16 @@
>
> static void cleanup_graphics_device(struct graphics_device *dev)
> {
> - free(dev->type);
> - free(dev->port);
> - free(dev->host);
> - free(dev->keymap);
> - free(dev->passwd);
> + if (dev->type !=NULL)
> + free(dev->type);
> + if (dev->port !=NULL)
> + free(dev->port);
> + if (dev->host !=NULL)
> + free(dev->host);
> + if (dev->keymap !=NULL)
> + free(dev->keymap);
> + if (dev->passwd !=NULL)
> + free(dev->passwd);
> }
>

I am not sure if those checks are really necessary. Isn't free(NULL) 
handled by glibc? One thing that could definitely cause a crash is 
having the dev pointer assigned to NULL.

> static void cleanup_input_device(struct input_device *dev)
> @@ -529,6 +534,13 @@
> if (gdev->port == NULL || gdev->host == NULL)
> goto err;
> }
> + else if (STREQC(gdev->type, "sdl")) {
> + SDL_display(gdev) = get_attr_value(node, "display");
> + SDL_xauth(gdev) = get_attr_value(node, "xauth");
> + SDL_fullscreen(gdev) = get_attr_value(node, "fullscreen");
> + gdev->passwd = NULL;
> + }
> +
> else if (STREQC(gdev->type, "pty")) {
> if (node->name == NULL)
> goto err;
> diff -r 395f2d684c10 -r ec5e5be04391 libxkutil/device_parsing.h
> --- a/libxkutil/device_parsing.h Tue Jul 05 15:52:31 2011 -0300
> +++ b/libxkutil/device_parsing.h Sun Jul 10 12:18:38 2011 +0800
> @@ -83,6 +83,9 @@
> char *path;
> };
>
> +#define SDL_display(dev) (dev->port)
> +#define SDL_xauth(dev) (dev->host)
> +#define SDL_fullscreen(dev) (dev->keymap)

You see, this is a decision you should have explained in the commit 
message. Anyway, I really don't like the idea of reusing the struct 
fields for another completely different purpose. Imagine when another 
type of graphic device lands. Wouldn't it be better to describe each 
graphic device as union? Code will look a lot more cleaner, less error 
prone and easier to maintain.

> struct graphics_device {
> char *type;
> char *port;
> diff -r 395f2d684c10 -r ec5e5be04391 libxkutil/xmlgen.c
> --- a/libxkutil/xmlgen.c Tue Jul 05 15:52:31 2011 -0300
> +++ b/libxkutil/xmlgen.c Sun Jul 10 12:18:38 2011 +0800
> @@ -421,8 +421,21 @@
>
> xmlNewProp(tmp, BAD_CAST "type", BAD_CAST dev->type);
>
> - if (STREQC(dev->type, "sdl"))
> - return NULL;
> + if (STREQC(dev->type, "sdl")) {
> + if (SDL_display(dev)) {
> + xmlNewProp(tmp, BAD_CAST "display",
> + BAD_CAST SDL_display(dev));
> + }
> + if (SDL_xauth(dev)) {
> + xmlNewProp(tmp, BAD_CAST "xauth",
> + BAD_CAST SDL_xauth(dev));
> + }
> + if (SDL_fullscreen(dev)) {
> + xmlNewProp(tmp, BAD_CAST "fullscreen",
> + BAD_CAST SDL_fullscreen(dev));
> + }
> + return NULL;
> + }
>
> if (dev->port) {
> xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->port);
> diff -r 395f2d684c10 -r ec5e5be04391
> schema/ResourceAllocationSettingData.mof
> --- a/schema/ResourceAllocationSettingData.mof Tue Jul 05 15:52:31 2011
> -0300
> +++ b/schema/ResourceAllocationSettingData.mof Sun Jul 10 12:18:38 2011
> +0800

What is this change from 0300 to 0800??

> @@ -219,7 +219,9 @@
> [Description ("If ResourceSubType is 'vnc', this is a VNC Address. "
> "IPv4 in a.b.c.d:port or IPv6 in [ip]:port format. If
> ResourceSubType "
> "is 'console', this is a character device path in "
> - "path:port format (e.g., '/dev/pts/3:0'\)")]
> + "path:port format (e.g., '/dev/pts/3:0'\)."
> + "if ResourceSubType is 'sdl', this is a combination of its
> params as "
> + "xauth:display (e.g., '/root/.Xauthority::0'\)")]
> string Address;
>
> [Description ("Keyboard keymapping")]
> @@ -228,7 +230,8 @@
> [Description ("VNC password")]
> string Password;
>
> - [Description ("Is IPv6 only addressing is to be used")]
> + [Description ("Is IPv6 only addressing is to be used."
> + "if ResourceSubType is 'sdl', this means whether sdl is fullscreen")]
> boolean IsIPv6Only;
> };
>
> diff -r 395f2d684c10 -r ec5e5be04391 src/Virt_RASD.c
> --- a/src/Virt_RASD.c Tue Jul 05 15:52:31 2011 -0300
> +++ b/src/Virt_RASD.c Sun Jul 10 12:18:38 2011 +0800
> @@ -416,12 +416,14 @@
> virDomainPtr dom = NULL;
> struct infostore_ctx *infostore = NULL;
> bool has_passwd = false;
> -
> + const struct graphics_device *gdev = &dev->dev.graphics;
> +
> CMSetProperty(inst, "ResourceSubType",
> (CMPIValue *)dev->dev.graphics.type, CMPI_chars);
>
> - if (STREQC(dev->dev.graphics.type, "sdl"))
> - rc = asprintf(&addr_str, "%s", dev->dev.graphics.type);
> + if (STREQC(dev->dev.graphics.type, "sdl")) {
> + rc = asprintf(&addr_str, "%s:%s", SDL_xauth(gdev),
> SDL_display(gdev));
> + }
> else {
> rc = asprintf(&addr_str,
> "%s:%s",
> diff -r 395f2d684c10 -r ec5e5be04391
> src/Virt_VirtualSystemManagementService.c
> --- a/src/Virt_VirtualSystemManagementService.c Tue Jul 05 15:52:31 2011
> -0300
> +++ b/src/Virt_VirtualSystemManagementService.c Sun Jul 10 12:18:38 2011
> +0800

Another one...

> @@ -1059,6 +1059,52 @@
> return ret;
> }
>
> +static int parse_sdl_address(const char *id,
> + char **display,
> + char **xauth)
> +{
> + int ret;
> + char *tmp_display = NULL;
> + char *tmp_xauth = NULL;
> +
> + CU_DEBUG("Entering parse_sdl_address, address is %s", id);
> +
> + ret = sscanf(id, "%a[^:]:%as", &tmp_xauth, &tmp_display);
> +
> + if (ret <= 0) {
> + ret = sscanf(id, ":%as", &tmp_display);
> + if (ret <= 0) {
> + if (STREQC(id, ":")) {
> + /* do nothing, it is empty */
> + }
> + else {
> + ret = 0;
> + goto out;
> + }
> + }
> + }
> +
> + if (display) {
> + if (tmp_display == NULL)
> + *display = NULL;
> + else
> + *display = strdup(tmp_display);
> + }
> + if (xauth) {
> + if (tmp_xauth == NULL)
> + *xauth = NULL;
> + else
> + *xauth = strdup(tmp_xauth);
> + }
> + ret = 1;
> +
> + out:
> + CU_DEBUG("Exiting parse_sdl_address, display is %s, xauth is %s",
> + *display, *xauth);
> +
> + return ret;
> +}
> +
> static int parse_vnc_address(const char *id,
> char **ip,
> char **port)
> @@ -1103,6 +1149,7 @@
> const char *msg = NULL;
> bool ipv6 = false;
> int ret;
> + struct graphics_device *gdev = &dev->dev.graphics;
>
> if (cu_get_str_prop(inst, "ResourceSubType", &val) !=
> CMPI_RC_OK) {
> msg = "GraphicsRASD ResourceSubType field not valid";
> @@ -1162,6 +1209,30 @@
> msg = "GraphicsRASD field Address not valid";
> goto out;
> }
> + }
> + else if (STREQC(dev->dev.graphics.type, "sdl")) {
> + if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK) {
> + CU_DEBUG("sdl graphics Address empty, using
> default");
> + SDL_display(gdev) = NULL;
> + SDL_xauth(gdev) = NULL;
> + }
> + else {
> + ret = parse_sdl_address(val,
> + &SDL_display(gdev),
> + &SDL_xauth(gdev));
> + if (ret != 1) {
> + msg = "GraphicsRASD sdl Address not
> valid";
> + goto out;
> + }
> + }
> + SDL_fullscreen(gdev) = NULL;
> + if (cu_get_bool_prop(inst, "IsIPV6Only", &ipv6) ==
> + CMPI_RC_OK) {
> + if (ipv6)
> + SDL_fullscreen(gdev) =
> strdup("yes");
> + else
> + SDL_fullscreen(gdev) =
> strdup("no");
> + }
> } else {
> CU_DEBUG("Unsupported graphics type %s",
> dev->dev.graphics.type);
> @@ -1172,6 +1243,9 @@
> free(dev->id);
> if (STREQC(dev->dev.graphics.type, "vnc"))
> ret = asprintf(&dev->id, "%s", dev->dev.graphics.type);
> + else if (STREQC(dev->dev.graphics.type, "sdl"))
> + ret = asprintf(&dev->id, "%s:%s:%s",
> + dev->dev.graphics.type, SDL_xauth(gdev),
> SDL_display(gdev));
> else
> ret = asprintf(&dev->id, "%s:%s",
> dev->dev.graphics.type,
> dev->dev.graphics.port);
> @@ -1493,7 +1567,6 @@
> "Failed to lookup resulting system");
> goto out;
> }
> -
> out:
> virDomainFree(dom);
> virConnectClose(conn);

Best regards,

-- 
Eduardo de Barros Lima
Software Engineer, Open Virtualization
Linux Technology Center - IBM/Brazil
eblima at br.ibm.com