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

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

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


-- 
Eduardo de Barros Lima
Software Engineer, Open Virtualization
Linux Technology Center - IBM/Brazil
eblima at br.ibm.com