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

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&gt; >> # 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&gt; >> >> 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, >