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

[Chip Vincent]

On 07/18/2011 03:54 AM, Wayne Xia wrote:
> 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.

Be sure the comment above is present when the patch is resubmitted

>
> 于 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.

Please do. The null check before delete is noisy :)

>
>>> 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.
>

I think the union approach is the way to go, even if it impacts existing 
code. The change is mostly a cut and paste and occurs in relatively few 
places in the code.

>>> 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,
>>
>
>

-- 
Chip Vincent
Open Virtualization
IBM Linux Technology Center
cvincent at linux.vnet.ibm.com