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

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

> >> >> 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(""); >> + }

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

> Best regards, Etrunko >