many thanks for your response.
some description:
Currently the SDL frame buffer is still supported by qemu and
libvirt, and I like to use it more than VNC. :)
https://bugzilla.linux.ibm.com/show_bug.cgi?id=71347
what libvirt-cim concern is to correctly pass the user definition about
it to libvirt, as parameters.
following is my comments.
于 2011-7-15 23:14, Eduardo Lima (Etrunko) 写道:
On 07/14/2011 03:32 AM, Wayne Xia wrote:
> # HG changeset patch
> # User Wayne Xia <xiawenc(a)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(a)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.
a crash happened before I added this code, more investigation need to
do about it.
> 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.
I agree this is a bad style.
But when I tried to use union, it seems the structure declaration need
to be changed too much making old code not compatible. For eg,
if graphics_device is declare as this:
union graphics_device{
struct vnc_device{
....
}dev1;
struct sdl_device{
....
}deve;
}graphics_device1;
then in code:
struct graphics_device* dev= *list;
dev->type = 0;
code like this would all need to be changed. it seems an additional
layer of member need to be added, if union is used.
> 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??
I thought it is auto generated by the HG, isn't it?
> @@ -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;
> };
>
here SDL used same attribute name For other graphic device, to minimize
the change in schema.
> 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,
--
Best Regards
Wayne Xia
mail:xiawenc@linux.vnet.ibm.com
tel:86-010-82450803