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

Mon Jul 18 07:54:04 UTC 2011

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 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.
>
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 at linux.vnet.ibm.com
tel:86-010-82450803