>I think a follow-up tweak may be order. For example,
I'd prefer to 

>simply use NULL for 'no media' rather than a null_string to simplify
the 

>code further.
Tests for this have completed successfully, so ACK'ing.
However...

Now that we have a cimtest patch for this 

(
https://www.redhat.com/archives/libvirt-cim/2011-September/msg00047.html),

I think a follow-up tweak may be order. For example, I'd prefer to 

simply use NULL for 'no media' rather than a null_string to simplify the

code further. We should also already have the libvirt-cim and Pegasus 

patches (no references, sorry) that allow us to un-set attributes like

this. That is, set an actual value to NULL.

On 09/20/2011 11:14 PM, Wayne Xia wrote:

> # HG changeset patch

> # User Wayne Xia<xiawenc@linux.vnet.ibm.com>

> # Date 1316154275 -28800

> # Node ID afee8d9b7214884ab74690b3ca9fd3d4f139f455

> # Parent  db809376d763493849c2a19f587969eaec619b75

> (#2) Fix the problem that libvirt-cim can't find cdrom device that
do not have disk

>

> This patch would allow define a system with an 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.

>

> (#2) Add comments that saying what the code does, and improved some
codes to avoid doing duplicated things.

>

> Signed-off-by: Wayne Xia<xiawenc@linux.vnet.ibm.com>

>

> diff -r db809376d763 -r afee8d9b7214 libxkutil/device_parsing.c

> --- a/libxkutil/device_parsing.c        
        Thu Jul 28 13:56:00 2011 -0300

> +++ b/libxkutil/device_parsing.c        
        Fri Sep 16 14:24:35 2011 +0800

> @@ -287,6 +287,13 @@

>                    
      ddev->shareable = true;

>                   }

>           }

> +

> +        /* handle the situation that a cdrom
device have no disk in it, no ISO file */

> +        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 db809376d763 -r afee8d9b7214 libxkutil/xmlgen.c

> --- a/libxkutil/xmlgen.c          
      Thu Jul 28 13:56:00 2011 -0300

> +++ b/libxkutil/xmlgen.c          
      Fri Sep 16 14:24:35 2011 +0800

> @@ -110,10 +110,18 @@

>                    
      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, ""))) {

> +                /* This is
the situation that user defined a cdrom device without

> +                 disk in
it, so skip generating a line saying "source", for that

> +                 xml defination
for libvirt should not have this defined in this

> +                 situation.
*/

> +        } 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 db809376d763 -r afee8d9b7214 src/Virt_VirtualSystemManagementService.c

> --- a/src/Virt_VirtualSystemManagementService.c      
          Thu Jul 28 13:56:00
2011 -0300

> +++ b/src/Virt_VirtualSystemManagementService.c      
          Fri Sep 16 14:24:35
2011 +0800

> @@ -879,6 +879,17 @@

>                   dev->dev.disk.device
= strdup("disk");

>           else if (type == VIRT_DISK_TYPE_CDROM)
{

>                   dev->dev.disk.device
= strdup("cdrom");

> +                /* following
code is for the case that user defined cdrom device

> +                   without
disk in it, or a empty disk "" */

> +                if (XSTREQ(dev->dev.disk.source,
"")) {

> +                    
   dev->dev.disk.disk_type = DISK_FILE;

> +                }

> +                if (XSTREQ(dev->dev.disk.source,
"/dev/null")) {

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

>           }

>

> _______________________________________________

> Libvirt-cim mailing list

> Libvirt-cim@redhat.com

> 
https://www.redhat.com/mailman/listinfo/libvirt-cim

-- 

Chip Vincent

Open Virtualization

IBM Linux Technology Center

cvincent@linux.vnet.ibm.com

_______________________________________________

Libvirt-cim mailing list

Libvirt-cim@redhat.com

https://www.redhat.com/mailman/listinfo/libvirt-cim