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

[Wayne Xia]

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
>


-- 
Best Regards

Wayne Xia
mail:xiawenc at linux.vnet.ibm.com
tel:86-010-82450803