于 2011-9-20 20:46, Eduardo Lima (Etrunko) 写道:
On 09/19/2011 11:47 PM, Wayne Xia wrote:
Thanks for the comments.
于 2011-9-20 4:33, Eduardo Lima (Etrunko) 写道:
On 09/16/2011 03:58 AM, Wayne Xia wrote: [snip]
diff -r fb09136deb49 -r 04b73e1cdc30 libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Tue Aug 30 08:48:36 2011 -0700 +++ b/libxkutil/xmlgen.c Fri Sep 16 14:24:35 2011 +0800 @@ -110,10 +110,15 @@ xmlNewProp(tmp, BAD_CAST "cache", BAD_CAST dev->cache); }
- tmp = xmlNewChild(disk, NULL, BAD_CAST "source", NULL); - if (tmp == NULL) - return XML_ERROR; - xmlNewProp(tmp, BAD_CAST "file", BAD_CAST dev->source); + if ((XSTREQ(dev->device, "cdrom"))&& + (XSTREQ(dev->source, ""))) { + /* do nothing */ + } else { + tmp = xmlNewChild(disk, NULL, BAD_CAST "source", NULL); + if (tmp == NULL) + return XML_ERROR; + xmlNewProp(tmp, BAD_CAST "file", BAD_CAST dev->source); + }
Well, I don't really like these types of conditionals where you actually only do something in the else block, especially because you can always achieve the same results by denying things.
Maybe in this style it tells clearly the code exclude the situation that device is a "cdrom" without "disk".
So maybe it is the case to let it explicit comment block.
I think comments would be good, but code like this: if ((!(XSTREQ(dev->device, "cdrom"))) || (!(XSTREQ(dev->source, "")))) { ...... } looks not so nice. Maybe keep the "if else" here and add comments would be better.
tmp = xmlNewChild(disk, NULL, BAD_CAST "target", NULL); if (tmp == NULL) diff -r fb09136deb49 -r 04b73e1cdc30 src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c Tue Aug 30 08:48:36 2011 -0700 +++ b/src/Virt_VirtualSystemManagementService.c Fri Sep 16 14:24:35 2011 +0800 @@ -879,6 +879,12 @@ dev->dev.disk.device = strdup("disk"); else if (type == VIRT_DISK_TYPE_CDROM) { dev->dev.disk.device = strdup("cdrom"); + if ((XSTREQ(dev->dev.disk.source, "/dev/null")) || + (XSTREQ(dev->dev.disk.source, ""))) { + dev->dev.disk.disk_type = DISK_FILE; + free(dev->dev.disk.source); + dev->dev.disk.source = strdup(""); + }
I have only seen this now, but it could be improved in the case of source being empty. Now you free and strdup it to the same value.
changed to avoid strdup the same value.
if (dev->dev.disk.disk_type == DISK_UNKNOWN) dev->dev.disk.disk_type = DISK_PHY; }
All in all the patch delivers the funcionality, so if you fix that logic I give you a +1.
This patch tried to provide libvirt-cim the capability to show instance of empty cdrom, so it used many "if" , tried consider only the situation about cdrom and its disk, to avoid effecting other functionalities.
I understand, but IMO, we should always try to follow the "keep it simple" principle, with clearer code and also as performatic as possible.
Best regards, Etrunko
-- Best Regards Wayne Xia mail:xiawenc@linux.vnet.ibm.com tel:86-010-82450803