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

On 07/26/2011 10:01 PM, Wayne Xia wrote: > # HG changeset patch > # User Wayne Xia<xiawenc(a)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(a)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.

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