3:11 p.m.
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.
3:11 p.m.
New subject: [PATCH 1 of 2] Add an EmulatedType field to DiskRASD to select CDROM or Disk
# HG changeset patch
# User Dan Smith <danms(a)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(a)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);
4:29 p.m.
New subject: [PATCH 1 of 2] Add an EmulatedType field to DiskRASD to select CDROM or Disk
Dan Smith wrote:
# HG changeset patch
# User Dan Smith <danms(a)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(a)linux.vnet.ibm.com
4:31 p.m.
New subject: [PATCH 1 of 2] Add an EmulatedType field to DiskRASD to select CDROM or Disk
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(a)us.ibm.com
10:38 a.m.
New subject: [PATCH 1 of 2] Add an EmulatedType field to DiskRASD to select CDROM or Disk
Dan Smith wrote:
# HG changeset patch
# User Dan Smith <danms(a)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
11:21 a.m.
New subject: [PATCH 1 of 2] Add an EmulatedType field to DiskRASD to select CDROM or Disk
Dan Smith wrote:
# HG changeset patch
# User Dan Smith <danms(a)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(a)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
3:02 p.m.
New subject: [PATCH 1 of 2] Add an EmulatedType field to DiskRASD to select CDROM or Disk
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(a)us.ibm.com
3:11 p.m.
New subject: [PATCH 2 of 2] Don't drop disk type on redefine
# HG changeset patch
# User Dan Smith <danms(a)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(a)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/>" : "",
4:38 p.m.
New subject: [PATCH 2 of 2] Don't drop disk type on redefine
Dan Smith wrote:
# HG changeset patch
# User Dan Smith <danms(a)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(a)linux.vnet.ibm.com
11:22 a.m.
New subject: [PATCH 2 of 2] Don't drop disk type on redefine
Dan Smith wrote:
# HG changeset patch
# User Dan Smith <danms(a)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
5842
days inactive
5843
days old
9 comments
3 participants
participants (3)
-
Dan Smith -
Jim Fehlig -
Kaitlin Rupert