Re: [Libvirt-cim] [PATCH] Fix the problem that libvirt-cim can't find cdrom device that do not have disk

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.

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

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