[PATCH 0 of 2] CDROM Support
This is not how I envisioned (or started to implement) CDROM support, but I think this is the better way. The second patch is the bit I sent before that makes xmlgen not clobber the device property. The new first patch adds the ability to set this in the Disk RASD on create, which gives us our CDROM support. If this is okay with people, I can follow it up with making Virt_Device expose a KVM_CDROMDrive instead of KVM_LogicalDisk for devices so configured.
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1226005676 28800 # Node ID 049f0bb012190e680257d00463138391405a5c60 # Parent 9385e61cd401162bef9c44bc11f64ca349a41abf Add an EmulatedType field to DiskRASD to select CDROM or Disk This seems like a pretty reasonable way to do this, but comments are welcome. I had initially planned to have a specific RASD type to represent a CDROM, but I don't think that makes much sense, and is significantly more complex. Adding this gives us a way to set and query the CDROM-ness of a disk, and with the following patch, avoids dropping this qualifier from existing configurations. Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 9385e61cd401 -r 049f0bb01219 schema/ResourceAllocationSettingData.mof --- a/schema/ResourceAllocationSettingData.mof Thu Nov 06 09:24:24 2008 -0800 +++ b/schema/ResourceAllocationSettingData.mof Thu Nov 06 13:07:56 2008 -0800 @@ -21,6 +21,10 @@ [Description ("Device as seen by the guest")] string VirtualDevice; + [Description ("Device emulation type"), + ValueMap {"0", "1"}, + Values {"Disk", "CDROM"}] + uint16 EmulatedType; }; [Description ("KVM virtual disk configuration"), @@ -32,6 +36,10 @@ [Description ("Device as seen by the guest")] string VirtualDevice; + [Description ("Device emulation type"), + ValueMap {"0", "1"}, + Values {"Disk", "CDROM"}] + uint16 EmulatedType; }; [Description ("LXC virtual disk configuration"), diff -r 9385e61cd401 -r 049f0bb01219 src/Virt_RASD.c --- a/src/Virt_RASD.c Thu Nov 06 09:24:24 2008 -0800 +++ b/src/Virt_RASD.c Thu Nov 06 13:07:56 2008 -0800 @@ -224,6 +224,7 @@ CMPIInstance *inst) { uint64_t cap = 0; + uint16_t type; CMPIStatus s = {CMPI_RC_OK, NULL}; get_vol_size(broker, ref, dev->dev.disk.source, &cap); @@ -243,6 +244,20 @@ "Address", (CMPIValue *)dev->dev.disk.source, CMPI_chars); + + /* There's not much we can do here if we don't recognize the type, + * so it seems that assuming 'disk' is a reasonable default + */ + if ((dev->dev.disk.device != NULL) && + STREQ(dev->dev.disk.device, "cdrom")) + type = VIRT_DISK_TYPE_CDROM; + else + type = VIRT_DISK_TYPE_DISK; + + CMSetProperty(inst, + "EmulatedType", + (CMPIValue *)&type, + CMPI_uint16); return s; } diff -r 9385e61cd401 -r 049f0bb01219 src/Virt_RASD.h --- a/src/Virt_RASD.h Thu Nov 06 09:24:24 2008 -0800 +++ b/src/Virt_RASD.h Thu Nov 06 13:07:56 2008 -0800 @@ -22,6 +22,9 @@ #define __VIRT_RASD_H #include "device_parsing.h" + +#define VIRT_DISK_TYPE_DISK 0 +#define VIRT_DISK_TYPE_CDROM 1 char *rasd_to_xml(CMPIInstance *rasd); diff -r 9385e61cd401 -r 049f0bb01219 src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c Thu Nov 06 09:24:24 2008 -0800 +++ b/src/Virt_VirtualSystemManagementService.c Thu Nov 06 13:07:56 2008 -0800 @@ -402,6 +402,7 @@ struct virt_device *dev) { const char *val = NULL; + uint16_t type; if (cu_get_str_prop(inst, "VirtualDevice", &val) != CMPI_RC_OK) return "Missing `VirtualDevice' property"; @@ -415,6 +416,16 @@ free(dev->dev.disk.source); dev->dev.disk.source = strdup(val); dev->dev.disk.disk_type = disk_type_from_file(val); + + if (cu_get_u16_prop(inst, "EmulatedType", &type) != CMPI_RC_OK) + type = VIRT_DISK_TYPE_DISK; + + if (type == VIRT_DISK_TYPE_DISK) + dev->dev.disk.device = strdup("disk"); + else if (type == VIRT_DISK_TYPE_CDROM) + dev->dev.disk.device = strdup("cdrom"); + else + return "Invalid value for EmulatedType"; free(dev->id); dev->id = strdup(dev->dev.disk.virtual_dev);
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1226005676 28800 # Node ID 049f0bb012190e680257d00463138391405a5c60 # Parent 9385e61cd401162bef9c44bc11f64ca349a41abf Add an EmulatedType field to DiskRASD to select CDROM or Disk
This seems like a pretty reasonable way to do this, but comments are welcome. I had initially planned to have a specific RASD type to represent a CDROM, but I don't think that makes much sense, and is significantly more complex. Adding this gives us a way to set and query the CDROM-ness of a disk, and with the following patch, avoids dropping this qualifier from existing configurations.
I like this approach. You'd mentioned in the previous set that you wanted the CDROM support to be an extension of the current disk support. I think this achieves this well. I don't think there's a reason to have a different CDROM device, since it's just an extension of the disk device. It similar to how a guest might have both a mouse and a tablet device (even though both are input devices). +1 -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
KR> I don't think there's a reason to have a different CDROM device, KR> since it's just an extension of the disk device. It similar to KR> how a guest might have both a mouse and a tablet device (even KR> though both are input devices). Well, I think a consolidated RASD is a good idea, but having the device itself as the appropriate class might help a GUI show the right icon, for example, for a given device. Anyway, we can do/decide that later. Thanks! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1226005676 28800 # Node ID 049f0bb012190e680257d00463138391405a5c60 # Parent 9385e61cd401162bef9c44bc11f64ca349a41abf Add an EmulatedType field to DiskRASD to select CDROM or Disk
This seems like a pretty reasonable way to do this, but comments are welcome. I had initially planned to have a specific RASD type to represent a CDROM, but I don't think that makes much sense, and is significantly more complex. Adding this gives us a way to set and query the CDROM-ness of a disk, and with the following patch, avoids dropping this qualifier from existing configurations.
Tested on Xen only, +1. Jim
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1226005676 28800 # Node ID 049f0bb012190e680257d00463138391405a5c60 # Parent 9385e61cd401162bef9c44bc11f64ca349a41abf Add an EmulatedType field to DiskRASD to select CDROM or Disk
This seems like a pretty reasonable way to do this, but comments are welcome. I had initially planned to have a specific RASD type to represent a CDROM, but I don't think that makes much sense, and is significantly more complex. Adding this gives us a way to set and query the CDROM-ness of a disk, and with the following patch, avoids dropping this qualifier from existing configurations.
Signed-off-by: Dan Smith <danms@us.ibm.com>
diff -r 9385e61cd401 -r 049f0bb01219 schema/ResourceAllocationSettingData.mof --- a/schema/ResourceAllocationSettingData.mof Thu Nov 06 09:24:24 2008 -0800 +++ b/schema/ResourceAllocationSettingData.mof Thu Nov 06 13:07:56 2008 -0800 @@ -21,6 +21,10 @@ [Description ("Device as seen by the guest")] string VirtualDevice;
+ [Description ("Device emulation type"), + ValueMap {"0", "1"}, + Values {"Disk", "CDROM"}] + uint16 EmulatedType; };
[Description ("KVM virtual disk configuration"), @@ -32,6 +36,10 @@ [Description ("Device as seen by the guest")] string VirtualDevice;
+ [Description ("Device emulation type"), + ValueMap {"0", "1"}, + Values {"Disk", "CDROM"}] + uint16 EmulatedType; };
After thinking about this more (and taking another look at CIM_RASD), couldn't the existing ResourceType CD Drive or DVD drive be leveraged instead of adding a new property? Jim
JF> After thinking about this more (and taking another look at JF> CIM_RASD), couldn't the existing ResourceType CD Drive or DVD JF> drive be leveraged instead of adding a new property? I went down this path a few weeks ago, which ended in frustration and patches sitting in my tree for a while. Because of the way we correlate things, we end up having to change the implementation of a lot of bits to handle that new ResourceType all the way through the stack. Every time I tried to compress Disk,CD to Disk, I'd find something further down the stack that expected them to be separate. ResourcePools, Devices, etc. Now, that's not a good justification for deciding how the model is designed, for sure, and it's definitely doable. If the preference is for this method, then I can revert the patch and we can go for the more heavy-weight option. I probably won't have time to finish my half-complete set for a while though. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1226005678 28800 # Node ID 9d100a5442150eedf372598f5033db100f47eddd # Parent 049f0bb012190e680257d00463138391405a5c60 Don't drop disk type on redefine Right now, we track the status of disk/cdrom of a disk device, but don't use it in the XML define. This patch uses the value instead of a static device='disk'. Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 049f0bb01219 -r 9d100a544215 libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Thu Nov 06 13:07:56 2008 -0800 +++ b/libxkutil/xmlgen.c Thu Nov 06 13:07:58 2008 -0800 @@ -119,12 +119,13 @@ int ret; ret = asprintf(&xml, - "<disk type='block' device='disk'>\n" + "<disk type='block' device='%s'>\n" " <source dev='%s'/>\n" " <target dev='%s'/>\n" "%s" "%s" "</disk>\n", + dev->device, dev->source, dev->virtual_dev, dev->readonly ? "<readonly/>\n" : "", @@ -141,12 +142,13 @@ int ret; ret = asprintf(&xml, - "<disk type='file' device='disk'>\n" + "<disk type='file' device='%s'>\n" " <source file='%s'/>\n" " <target dev='%s'/>\n" "%s" "%s" "</disk>\n", + dev->device, dev->source, dev->virtual_dev, dev->readonly ? "<readonly/>" : "",
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1226005678 28800 # Node ID 9d100a5442150eedf372598f5033db100f47eddd # Parent 049f0bb012190e680257d00463138391405a5c60 Don't drop disk type on redefine
Right now, we track the status of disk/cdrom of a disk device, but don't use it in the XML define. This patch uses the value instead of a static device='disk'.
+1 -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1226005678 28800 # Node ID 9d100a5442150eedf372598f5033db100f47eddd # Parent 049f0bb012190e680257d00463138391405a5c60 Don't drop disk type on redefine
Right now, we track the status of disk/cdrom of a disk device, but don't use it in the XML define. This patch uses the value instead of a static device='disk'.
Tested on xen only, +1. Jim
participants (3)
-
Dan Smith -
Jim Fehlig -
Kaitlin Rupert