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

[Wayne Xia]

于 2011-9-20 20:46, 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.
>
I think comments would be good, but code like this:

if ((!(XSTREQ(dev->device, "cdrom"))) || 
(!(XSTREQ(dev->source, "")))) {
           ......
}

looks not so nice. Maybe keep the "if else" here and add comments would
be better.

>>
>>>
>>>>
>>>>            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.
>
changed to avoid strdup 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
>>>
>>
>>
>
>


-- 
Best Regards

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