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

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.

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

Best regards, Etrunko