8:58 a.m.
# HG changeset patch
# User Wayne Xia <xiawenc(a)linux.vnet.ibm.com>
# Date 1316154275 -28800
# Node ID 04b73e1cdc3087a390b9790c13b42ebacbc5d5ba
# Parent fb09136deb494008eb3aacee420ad74da7d7c294
Fix the problem that libvirt-cim can't find cdrom device that do not have disk
This patch would allow define a system with a empty CDROM device, and allow method modify
resource settings to insert ISO files into an empty CDROM device.
Examples:
InvokeMethod(ModifyResourceSettings):
ResourceSettings: ['instance of KVM_DiskResourceAllocationSettingData {\nResourceType
= 17;\nInstanceID = "test/hdc";\nEmulatedType = 1;\nVirtualDevice =
"hdc";\nAddress = "";\n};']
InvokeMethod(ModifyResourceSettings):
ResourceSettings: ['instance of KVM_DiskResourceAllocationSettingData {\nResourceType
= 17;\nInstanceID = "test/hdc";\nEmulatedType = 1;\nVirtualDevice =
"hdc";\nAddress = "/var/lib/libvirt/images/test-disk.iso";\n};']
Note that the Address property should be set to "", not None(not set), to tell
that user want
an ejection.
Signed-off-by: Wayne Xia <xiawenc(a)linux.vnet.ibm.com>
diff -r fb09136deb49 -r 04b73e1cdc30 libxkutil/device_parsing.c
--- a/libxkutil/device_parsing.c Tue Aug 30 08:48:36 2011 -0700
+++ b/libxkutil/device_parsing.c Fri Sep 16 14:24:35 2011 +0800
@@ -287,6 +287,12 @@
ddev->shareable = true;
}
}
+
+ if ((XSTREQ(ddev->device, "cdrom")) && (ddev->source ==
NULL)) {
+ ddev->source = strdup("");
+ ddev->disk_type = DISK_FILE;
+ }
+
if ((ddev->source == NULL) || (ddev->virtual_dev == NULL))
goto err;
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);
+ }
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;
}
10:33 p.m.
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
--
Eduardo de Barros Lima
Software Engineer, Open Virtualization
Linux Technology Center - IBM/Brazil
eblima(a)br.ibm.com
4:47 a.m.
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@linux.vnet.ibm.com
tel:86-010-82450803
2:46 p.m.
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(a)br.ibm.com
5:10 a.m.
于 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@linux.vnet.ibm.com
tel:86-010-82450803
4744
days inactive
4749
days old
4 comments
2 participants
participants (2)
-
Eduardo Lima (Etrunko) -
Wayne Xia