[Libvirt-cim] [PATCH] (#3) made the graphic structure as union

[Wayne Xia]

于 2011-7-29 2:24, Eduardo Lima (Etrunko) 写道:
> On 07/26/2011 10:01 PM, Wayne Xia wrote:
>> # HG changeset patch
>> # User Wayne Xia<xiawenc at linux.vnet.ibm.com>
>> # Date 1311593948 -28800
>> # Node ID 792db1a6ead075375fad4a7d22143a0f978b5e48
>> # Parent 0f42cab9c45c53cc13407b16418399ed8ed4a026
>> (#3) made the graphic structure as union
>>
>> These change were made to allow SDL device properties added more clearly
>>
>> Signed-off-by: Wayne Xia (Wayne)<xiawenc at linux.vnet.ibm.com>
>>
>> diff -r 0f42cab9c45c -r 792db1a6ead0 libxkutil/device_parsing.c
>> --- a/libxkutil/device_parsing.c Mon Jul 25 13:14:22 2011 -0700
>> +++ b/libxkutil/device_parsing.c Mon Jul 25 19:39:08 2011 +0800
>> @@ -91,10 +91,10 @@
>> static void cleanup_graphics_device(struct graphics_device *dev)
>> {
>> free(dev->type);
>> - free(dev->port);
>> - free(dev->host);
>> - free(dev->keymap);
>> - free(dev->passwd);
>> + free(dev->dev.vnc.port);
>> + free(dev->dev.vnc.host);
>> + free(dev->dev.vnc.keymap);
>> + free(dev->dev.vnc.passwd);
>> }
>>
>
> Hi Wayne, sorry for the delay.
>
> I know that the compiler would take care of this, but just to follow the
> 'convention' present in other places of the code, whenever you have an
> union, please provide a dedicate cleanup function for each structure.
> You can take a look on pool_parsing for example:
>
> if (dev->type == sdl)
> clenup_sdl_device();
> else if (dev->type = vnc)
> cleanup_vnc_device();
>
> Also, it won't show in this email but I see a few places where your
> patch introduces trailing whitespaces. If you happen to use vim, you can
> make it show them with the following setting in your vimrc:
>
> highlight WhitespaceEOL ctermbg=red guibg=red
> match WhitespaceEOL /\s\+$\| \+\ze\t/
>
> With that you will notice there are many other places in the code which
> will get highlighted. Yeah, not everyone pay attention to those details,
> but I particularly like to keep the code clean. If you are changing a
> line with trailing whitespaces, feel free to change it and leave others
> as is. Hopefully we will fix that someday.
>
>
thanks for the tip, patch on the way

>> static void cleanup_input_device(struct input_device *dev)
>> @@ -530,12 +530,12 @@
>> CU_DEBUG("graphics device type = %s", gdev->type);
>>
>> if (STREQC(gdev->type, "vnc")) {
>> - gdev->port = get_attr_value(node, "port");
>> - gdev->host = get_attr_value(node, "listen");
>> - gdev->keymap = get_attr_value(node, "keymap");
>> - gdev->passwd = get_attr_value(node, "passwd");
>> + gdev->dev.vnc.port = get_attr_value(node, "port");
>> + gdev->dev.vnc.host = get_attr_value(node, "listen");
>> + gdev->dev.vnc.keymap = get_attr_value(node, "keymap");
>> + gdev->dev.vnc.passwd = get_attr_value(node, "passwd");
>>
>> - if (gdev->port == NULL || gdev->host == NULL)
>> + if (gdev->dev.vnc.port == NULL || gdev->dev.vnc.host == NULL)
>> goto err;
>> }
>> else if (STREQC(gdev->type, "pty")) {
>> @@ -550,9 +550,9 @@
>> for (child = node->children; child != NULL;
>> child = child->next) {
>> if (XSTREQ(child->name, "source"))
>> - gdev->host = get_attr_value(child, "path");
>> + gdev->dev.vnc.host = get_attr_value(child, "path");
>> else if (XSTREQ(child->name, "target"))
>> - gdev->port = get_attr_value(child, "port");
>> + gdev->dev.vnc.port = get_attr_value(child, "port");
>> }
>> }
>> else {
>> @@ -565,7 +565,7 @@
>> if (STREQC(gdev->type, "vnc"))
>> ret = asprintf(&vdev->id, "%s", gdev->type);
>> else
>> - ret = asprintf(&vdev->id, "%s:%s", gdev->type, gdev->port);
>> + ret = asprintf(&vdev->id, "%s:%s", gdev->type, gdev->dev.vnc.port);
>>
>> if (ret == -1) {
>> CU_DEBUG("Failed to create graphics is string");
>> @@ -806,10 +806,10 @@
>> DUP_FIELD(dev, _dev, dev.emu.path);
>> } else if (dev->type == CIM_RES_TYPE_GRAPHICS) {
>> DUP_FIELD(dev, _dev, dev.graphics.type);
>> - DUP_FIELD(dev, _dev, dev.graphics.port);
>> - DUP_FIELD(dev, _dev, dev.graphics.host);
>> - DUP_FIELD(dev, _dev, dev.graphics.keymap);
>> - DUP_FIELD(dev, _dev, dev.graphics.passwd);
>> + DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.host);
>> + DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.port);
>> + DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.keymap);
>> + DUP_FIELD(dev, _dev, dev.graphics.dev.vnc.passwd);
>> } else if (dev->type == CIM_RES_TYPE_INPUT) {
>> DUP_FIELD(dev, _dev, dev.input.type);
>> DUP_FIELD(dev, _dev, dev.input.bus);
>> diff -r 0f42cab9c45c -r 792db1a6ead0 libxkutil/device_parsing.h
>> --- a/libxkutil/device_parsing.h Mon Jul 25 13:14:22 2011 -0700
>> +++ b/libxkutil/device_parsing.h Mon Jul 25 19:39:08 2011 +0800
>> @@ -84,14 +84,28 @@
>> char *path;
>> };
>>
>> -struct graphics_device {
>> - char *type;
>> +//vnc_device must be larger or equal than sdl_device
>> +struct vnc_device {
>> char *port;
>> char *host;
>> char *keymap;
>> char *passwd;
>> };
>>
>> +struct sdl_device {
>> + char *display;
>> + char *xauth;
>> + char *fullscreen;
>> +};
>> +
>> +struct graphics_device {
>> + char *type;
>> + union {
>> + struct vnc_device vnc;
>> + struct sdl_device sdl;
>> + } dev;
>> +};
>> +
>> struct input_device {
>> char *type;
>> char *bus;
>> diff -r 0f42cab9c45c -r 792db1a6ead0 libxkutil/xml_parse_test.c
>> --- a/libxkutil/xml_parse_test.c Mon Jul 25 13:14:22 2011 -0700
>> +++ b/libxkutil/xml_parse_test.c Mon Jul 25 19:39:08 2011 +0800
>> @@ -116,7 +116,7 @@
>> FILE *d)
>> {
>> print_value(d, "Graphics Type", dev->dev.graphics.type);
>> - print_value(d, "Graphics Port", dev->dev.graphics.port);
>> + print_value(d, "Graphics Port", dev->dev.graphics.dev.vnc.port);
>> }
>>
>> static void print_devices(struct domain *dominfo,
>> diff -r 0f42cab9c45c -r 792db1a6ead0 libxkutil/xmlgen.c
>> --- a/libxkutil/xmlgen.c Mon Jul 25 13:14:22 2011 -0700
>> +++ b/libxkutil/xmlgen.c Mon Jul 25 19:39:08 2011 +0800
>> @@ -424,22 +424,22 @@
>> if (STREQC(dev->type, "sdl"))
>> return NULL;
>>
>> - if (dev->port) {
>> - xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->port);
>> - if (STREQC(dev->port, "-1"))
>> + if (dev->dev.vnc.port) {
>> + xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->dev.vnc.port);
>> + if (STREQC(dev->dev.vnc.port, "-1"))
>> xmlNewProp(tmp, BAD_CAST "autoport", BAD_CAST "yes");
>> else
>> xmlNewProp(tmp, BAD_CAST "autoport", BAD_CAST "no");
>> }
>>
>> - if (dev->host)
>> - xmlNewProp(tmp, BAD_CAST "listen", BAD_CAST dev->host);
>> + if (dev->dev.vnc.host)
>> + xmlNewProp(tmp, BAD_CAST "listen", BAD_CAST dev->dev.vnc.host);
>>
>> - if (dev->passwd)
>> - xmlNewProp(tmp, BAD_CAST "passwd", BAD_CAST dev->passwd);
>> + if (dev->dev.vnc.passwd)
>> + xmlNewProp(tmp, BAD_CAST "passwd", BAD_CAST dev->dev.vnc.passwd);
>>
>> - if (dev->keymap)
>> - xmlNewProp(tmp, BAD_CAST "keymap", BAD_CAST dev->keymap);
>> + if (dev->dev.vnc.keymap)
>> + xmlNewProp(tmp, BAD_CAST "keymap", BAD_CAST dev->dev.vnc.keymap);
>>
>> return NULL;
>> }
>> @@ -459,16 +459,16 @@
>> tmp = xmlNewChild(pty, NULL, BAD_CAST "source", NULL);
>> if (tmp == NULL)
>> return XML_ERROR;
>> -
>> - if(dev->host)
>> - xmlNewProp(tmp, BAD_CAST "path", BAD_CAST dev->host);
>> +
>> + if(dev->dev.vnc.host)
>> + xmlNewProp(tmp, BAD_CAST "path", BAD_CAST dev->dev.vnc.host);
>>
>> tmp = xmlNewChild(pty, NULL, BAD_CAST "target", NULL);
>> if (tmp == NULL)
>> return XML_ERROR;
>> -
>> - if(dev->port)
>> - xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->port);
>> +
>> + if(dev->dev.vnc.port)
>> + xmlNewProp(tmp, BAD_CAST "port", BAD_CAST dev->dev.vnc.port);
>>
>> return NULL;
>> }
>> diff -r 0f42cab9c45c -r 792db1a6ead0 src/Virt_ComputerSystem.c
>> --- a/src/Virt_ComputerSystem.c Mon Jul 25 13:14:22 2011 -0700
>> +++ b/src/Virt_ComputerSystem.c Mon Jul 25 19:39:08 2011 +0800
>> @@ -104,7 +104,7 @@
>> "Virtual System (Console on %s://%s:%s)",
>> domain->dev_graphics[0].dev.graphics.type,
>> host,
>> - domain->dev_graphics[0].dev.graphics.port);
>> + domain->dev_graphics[0].dev.graphics.dev.vnc.port);
>> else
>> ret = asprintf(&cap,
>> "Virtual System (No console)");
>> diff -r 0f42cab9c45c -r 792db1a6ead0 src/Virt_Device.c
>> --- a/src/Virt_Device.c Mon Jul 25 13:14:22 2011 -0700
>> +++ b/src/Virt_Device.c Mon Jul 25 19:39:08 2011 +0800
>> @@ -194,8 +194,8 @@
>> else
>> rc = asprintf(&vp_str, "%s/%s:%s",
>> dev->type,
>> - dev->host,
>> - dev->port);
>> + dev->dev.vnc.host,
>> + dev->dev.vnc.port);
>> if (rc == -1)
>> return 0;
>>
>> diff -r 0f42cab9c45c -r 792db1a6ead0 src/Virt_KVMRedirectionSAP.c
>> --- a/src/Virt_KVMRedirectionSAP.c Mon Jul 25 13:14:22 2011 -0700
>> +++ b/src/Virt_KVMRedirectionSAP.c Mon Jul 25 19:39:08 2011 +0800
>> @@ -366,7 +366,7 @@
>> continue;
>> }
>>
>> - ret = sscanf(dominfo->dev_graphics->dev.graphics.port,
>> + ret = sscanf(dominfo->dev_graphics->dev.graphics.dev.vnc.port,
>> "%d",
>> &lport);
>> if (ret != 1) {
>> diff -r 0f42cab9c45c -r 792db1a6ead0 src/Virt_RASD.c
>> --- a/src/Virt_RASD.c Mon Jul 25 13:14:22 2011 -0700
>> +++ b/src/Virt_RASD.c Mon Jul 25 19:39:08 2011 +0800
>> @@ -481,9 +481,9 @@
>> rc = asprintf(&addr_str, "%s", dev->dev.graphics.type);
>> else {
>> rc = asprintf(&addr_str,
>> - "%s:%s",
>> - dev->dev.graphics.host,
>> - dev->dev.graphics.port);
>> + "%s:%s",
>> + dev->dev.graphics.dev.vnc.host,
>> + dev->dev.graphics.dev.vnc.port);
>> }
>>
>> CU_DEBUG("graphics Address = %s", addr_str);
>> @@ -496,7 +496,7 @@
>>
>> if (STREQC(dev->dev.graphics.type, "vnc")) {
>> CMSetProperty(inst, "KeyMap",
>> - (CMPIValue *)dev->dev.graphics.keymap, CMPI_chars);
>> + (CMPIValue *)dev->dev.graphics.dev.vnc.keymap, CMPI_chars);
>>
>> conn = connect_by_classname(_BROKER, classname,&s);
>> if (conn == NULL)
>> @@ -511,7 +511,8 @@
>> goto out;
>> }
>>
>> - if (dev->dev.graphics.passwd&& strlen(dev->dev.graphics.passwd)) {
>> + if (dev->dev.graphics.dev.vnc.passwd&&
>> + strlen(dev->dev.graphics.dev.vnc.passwd)) {
>> CU_DEBUG("has password");
>> CMSetProperty(inst, "Password",
>> (CMPIValue *)"********", CMPI_chars);
>> diff -r 0f42cab9c45c -r 792db1a6ead0
>> src/Virt_VirtualSystemManagementService.c
>> --- a/src/Virt_VirtualSystemManagementService.c Mon Jul 25 13:14:22
>> 2011 -0700
>> +++ b/src/Virt_VirtualSystemManagementService.c Mon Jul 25 19:39:08
>> 2011 +0800
>> @@ -370,10 +370,10 @@
>> }
>>
>> domain->dev_graphics->dev.graphics.type = strdup("vnc");
>> - domain->dev_graphics->dev.graphics.port = strdup("-1");
>> - domain->dev_graphics->dev.graphics.host = strdup("127.0.0.1");
>> - domain->dev_graphics->dev.graphics.keymap = strdup("en-us");
>> - domain->dev_graphics->dev.graphics.passwd = NULL;
>> + domain->dev_graphics->dev.graphics.dev.vnc.port = strdup("-1");
>> + domain->dev_graphics->dev.graphics.dev.vnc.host = strdup("127.0.0.1");
>> + domain->dev_graphics->dev.graphics.dev.vnc.keymap = strdup("en-us");
>> + domain->dev_graphics->dev.graphics.dev.vnc.passwd = NULL;
>> domain->dev_graphics_ct = 1;
>>
>> return true;
>> @@ -1129,24 +1129,24 @@
>> }
>>
>> ret = parse_vnc_address(val,
>> -&dev->dev.graphics.host,
>> -&dev->dev.graphics.port);
>> +&dev->dev.graphics.dev.vnc.host,
>> +&dev->dev.graphics.dev.vnc.port);
>> if (ret != 1) {
>> msg = "GraphicsRASD field Address not valid";
>> goto out;
>> }
>>
>> if (cu_get_str_prop(inst, "KeyMap",&val) != CMPI_RC_OK)
>> - dev->dev.graphics.keymap = strdup("en-us");
>> + dev->dev.graphics.dev.vnc.keymap = strdup("en-us");
>> else
>> - dev->dev.graphics.keymap = strdup(val);
>> + dev->dev.graphics.dev.vnc.keymap = strdup(val);
>>
>> if (cu_get_str_prop(inst, "Password",&val) != CMPI_RC_OK) {
>> CU_DEBUG("vnc password is not set");
>> - dev->dev.graphics.passwd = NULL;
>> + dev->dev.graphics.dev.vnc.passwd = NULL;
>> } else {
>> CU_DEBUG("vnc password is set");
>> - dev->dev.graphics.passwd = strdup(val);
>> + dev->dev.graphics.dev.vnc.passwd = strdup(val);
>> }
>> }
>> else if (STREQC(dev->dev.graphics.type, "console") ||
>> @@ -1157,8 +1157,8 @@
>> }
>>
>> ret = parse_console_address(val,
>> -&dev->dev.graphics.host,
>> -&dev->dev.graphics.port);
>> +&dev->dev.graphics.dev.vnc.host,
>> +&dev->dev.graphics.dev.vnc.port);
>> if (ret != 1) {
>> msg = "GraphicsRASD field Address not valid";
>> goto out;
>> @@ -1175,7 +1175,7 @@
>> ret = asprintf(&dev->id, "%s", dev->dev.graphics.type);
>> else
>> ret = asprintf(&dev->id, "%s:%s",
>> - dev->dev.graphics.type, dev->dev.graphics.port);
>> + dev->dev.graphics.type, dev->dev.graphics.dev.vnc.port);
>>
>> if (ret == -1) {
>> msg = "Failed to create graphics is string";
>>
>
> I have applied and tested this patch. It works fine, please provide that
> small fix in the cleanup function and it will be ready.
>
> Best regards,
>


-- 
Best Regards

Wayne Xia
mail:xiawenc at linux.vnet.ibm.com
tel:86-010-82450803