4:31 p.m.
This patch set attempts to close the gap between what a client expects to be
able to discover and what we allow. After this, the client should be able
to use the default NetRASDs exposed from AllocationCapabilities as templates
for a DefineSystem operation with proper pool (i.e. Virtual Network) membership
maintained and unique MACs assigned.
Comments and thorough review appreciated :)
4:31 p.m.
New subject: [PATCH 1 of 6] Make the 'default' NetRASD allocation include a randomly-generated MAC
# HG changeset patch
# User Dan Smith <danms(a)us.ibm.com>
# Date 1207776499 25200
# Node ID d297967bad1fb290230fa9f42dee9a4e5e68f7bb
# Parent ad9a40f4e04defe60897169cd7b93ef115bfcaf8
Make the 'default' NetRASD allocation include a randomly-generated MAC
Signed-off-by: Dan Smith <danms(a)us.ibm.com>
diff -r ad9a40f4e04d -r d297967bad1f src/Virt_SettingsDefineCapabilities.c
--- a/src/Virt_SettingsDefineCapabilities.c Wed Apr 09 14:28:18 2008 -0700
+++ b/src/Virt_SettingsDefineCapabilities.c Wed Apr 09 14:28:19 2008 -0700
@@ -25,6 +25,7 @@
#include <stdbool.h>
#include <sys/vfs.h>
#include <errno.h>
+#include <time.h>
#include <libvirt/libvirt.h>
@@ -52,6 +53,8 @@ const static CMPIBroker *_BROKER;
#define SDC_DISK_MIN 2000
#define SDC_DISK_DEF 5000
#define SDC_DISK_INC 250
+
+#define DEFAULT_MAC_PREFIX "00:16:3e"
static bool rasd_prop_copy_value(struct sdc_rasd_prop src,
struct sdc_rasd_prop *dest)
@@ -430,6 +433,45 @@ static struct sdc_rasd_prop *net_max(con
return rasd;
}
+static const char *_net_rand_mac(void)
+{
+ int r;
+ int ret;
+ unsigned int s;
+ char *mac = NULL;
+ CMPIString *str = NULL;
+ CMPIStatus status;
+
+ srand(time(NULL));
+ r = rand_r(&s);
+
+ ret = asprintf(&mac,
+ "%s:%02x:%02x:%02x",
+ DEFAULT_MAC_PREFIX,
+ r & 0xFF,
+ (r & 0xFF00) >> 8,
+ (r & 0xFF0000) >> 16);
+
+ if (ret == -1)
+ goto out;
+
+ str = CMNewString(_BROKER, mac, &status);
+ if ((str == NULL) || (status.rc != CMPI_RC_OK)) {
+ str = NULL;
+ CU_DEBUG("Failed to create string");
+ goto out;
+ }
+ out:
+ free(mac);
+
+ if (str != NULL)
+ mac = CMGetCharPtr(str);
+ else
+ mac = NULL;
+
+ return mac;
+}
+
static struct sdc_rasd_prop *net_def(const CMPIObjectPath *ref,
CMPIStatus *s)
{
@@ -440,6 +482,7 @@ static struct sdc_rasd_prop *net_def(con
struct sdc_rasd_prop tmp[] = {
{"InstanceID", (CMPIValue *)"Default", CMPI_chars},
{"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16},
+ {"Address", (CMPIValue *)_net_rand_mac(), CMPI_chars},
PROP_END
};
4:31 p.m.
New subject: [PATCH 2 of 6] Set PoolID on all allocated RASD objects for later correlation in DefineSystem
# HG changeset patch
# User Dan Smith <danms(a)us.ibm.com>
# Date 1207776520 25200
# Node ID fae3c71425bf5d5adc01a6de1005c87b8726916c
# Parent d297967bad1fb290230fa9f42dee9a4e5e68f7bb
Set PoolID on all allocated RASD objects for later correlation in DefineSystem
Signed-off-by: Dan Smith <danms(a)us.ibm.com>
diff -r d297967bad1f -r fae3c71425bf src/Virt_SettingsDefineCapabilities.c
--- a/src/Virt_SettingsDefineCapabilities.c Wed Apr 09 14:28:19 2008 -0700
+++ b/src/Virt_SettingsDefineCapabilities.c Wed Apr 09 14:28:40 2008 -0700
@@ -829,6 +829,7 @@ static CMPIStatus alloc_cap_to_rasd(cons
CMPIStatus s = {CMPI_RC_OK};
uint16_t type;
const char *id = NULL;
+ int i;
if (!match_hypervisor_prefix(ref, info))
return s;
@@ -850,6 +851,10 @@ static CMPIStatus alloc_cap_to_rasd(cons
}
s = sdc_rasds_for_type(ref, list, type);
+
+ for (i = 0; i < list->cur; i++)
+ CMSetProperty(list->list[i], "PoolID",
+ (CMPIValue *)id, CMPI_chars);
out:
return s;
4:31 p.m.
New subject: [PATCH 3 of 6] Fix typo in NetRASD creation that could have caused a break if we rearrange things
# HG changeset patch
# User Dan Smith <danms(a)us.ibm.com>
# Date 1207776529 25200
# Node ID 17f2f0d424887bc0c51037a3bbc7fd665938e8ba
# Parent fae3c71425bf5d5adc01a6de1005c87b8726916c
Fix typo in NetRASD creation that could have caused a break if we rearrange things
Signed-off-by: Dan Smith <danms(a)us.ibm.com>
diff -r fae3c71425bf -r 17f2f0d42488 src/Virt_RASD.c
--- a/src/Virt_RASD.c Wed Apr 09 14:28:40 2008 -0700
+++ b/src/Virt_RASD.c Wed Apr 09 14:28:49 2008 -0700
@@ -155,7 +155,7 @@ static CMPIInstance *rasd_from_vdev(cons
} else if (dev->type == CIM_RES_TYPE_NET) {
CMSetProperty(inst,
"NetworkType",
- (CMPIValue *)dev->dev.disk.type,
+ (CMPIValue *)dev->dev.net.type,
CMPI_chars);
} else if (dev->type == CIM_RES_TYPE_MEM) {
const char *units = "KiloBytes";
4:31 p.m.
New subject: [PATCH 4 of 6] Make NetRASD expose its MAC in the Address field
# HG changeset patch
# User Dan Smith <danms(a)us.ibm.com>
# Date 1207776541 25200
# Node ID 0314f5ce51d0d8ba4d518c0a7a22c43e03be4dec
# Parent 17f2f0d424887bc0c51037a3bbc7fd665938e8ba
Make NetRASD expose its MAC in the Address field
Signed-off-by: Dan Smith <danms(a)us.ibm.com>
diff -r 17f2f0d42488 -r 0314f5ce51d0 src/Virt_RASD.c
--- a/src/Virt_RASD.c Wed Apr 09 14:28:49 2008 -0700
+++ b/src/Virt_RASD.c Wed Apr 09 14:29:01 2008 -0700
@@ -157,6 +157,10 @@ static CMPIInstance *rasd_from_vdev(cons
"NetworkType",
(CMPIValue *)dev->dev.net.type,
CMPI_chars);
+ CMSetProperty(inst,
+ "Address",
+ (CMPIValue *)dev->dev.net.mac,
+ CMPI_chars);
} else if (dev->type == CIM_RES_TYPE_MEM) {
const char *units = "KiloBytes";
4:31 p.m.
New subject: [PATCH 5 of 6] Add function to parse out pool name from InstanceID
# HG changeset patch
# User Dan Smith <danms(a)us.ibm.com>
# Date 1207776548 25200
# Node ID 3d143b851ee9201986ce8d0e6656cc313d361866
# Parent 0314f5ce51d0d8ba4d518c0a7a22c43e03be4dec
Add function to parse out pool name from InstanceID
Signed-off-by: Dan Smith <danms(a)us.ibm.com>
diff -r 0314f5ce51d0 -r 3d143b851ee9 src/Virt_DevicePool.c
--- a/src/Virt_DevicePool.c Wed Apr 09 14:29:01 2008 -0700
+++ b/src/Virt_DevicePool.c Wed Apr 09 14:29:08 2008 -0700
@@ -500,6 +500,17 @@ uint16_t res_type_from_pool_id(const cha
return CIM_RES_TYPE_UNKNOWN;
}
+char *name_from_pool_id(const char *id)
+{
+ char *s;
+
+ s = strchr(id, '/');
+ if (s == NULL)
+ return NULL;
+
+ return strdup((char *)s+1);
+}
+
static bool mempool_set_total(CMPIInstance *inst, virConnectPtr conn)
{
virNodeInfo info;
diff -r 0314f5ce51d0 -r 3d143b851ee9 src/Virt_DevicePool.h
--- a/src/Virt_DevicePool.h Wed Apr 09 14:29:01 2008 -0700
+++ b/src/Virt_DevicePool.h Wed Apr 09 14:29:08 2008 -0700
@@ -58,6 +58,14 @@ uint16_t res_type_from_pool_id(const cha
uint16_t res_type_from_pool_id(const char *id);
/**
+ * Get the pool name from a given pool's InstanceID
+ *
+ * @param id The InstanceID of the pool
+ * @returns the name (must be free'd by the caller)
+ */
+char *name_from_pool_id(const char *id);
+
+/**
* Get all device pools on the system for the given type
*
*
4:31 p.m.
New subject: [PATCH 6 of 6] Make VSMS not parse the InstanceID of a RASD, but rather use the proper fields
# HG changeset patch
# User Dan Smith <danms(a)us.ibm.com>
# Date 1207776680 25200
# Node ID 01e5b1388ee7604332c442cba79c39ac3cca0b6f
# Parent 3d143b851ee9201986ce8d0e6656cc313d361866
Make VSMS not parse the InstanceID of a RASD, but rather use the proper fields
For Network, grab the Address for the MAC and then also try to extract
the PoolID for the network name. I left it split between KVM and Xen,
because I think as we expand the supported network types, we'll still need
to differentiate in the code.
This means that we no longer examine the InstanceID of the incoming RASDs
for a DefineSystem operation (which is correct). The test suite will need
to change to use these values correctly instead of generating the InstanceID
and should also have a test that verifies that the InstanceID is not
honored. I think that the InstanceID should be ignored if specified,
although discussion of this point is certainly welcomed.
Signed-off-by: Dan Smith <danms(a)us.ibm.com>
diff -r 3d143b851ee9 -r 01e5b1388ee7 src/Makefile.am
--- a/src/Makefile.am Wed Apr 09 14:29:08 2008 -0700
+++ b/src/Makefile.am Wed Apr 09 14:31:20 2008 -0700
@@ -75,9 +75,9 @@ libVirt_ComputerSystemMigrationIndicatio
libVirt_ComputerSystemMigrationIndication_la_SOURCES =
Virt_ComputerSystemMigrationIndication.c
libVirt_ComputerSystemMigrationIndication_la_LIBADD = -lVirt_ComputerSystem
-libVirt_VirtualSystemManagementService_la_DEPENDENCIES = libVirt_ComputerSystem.la
libVirt_ComputerSystemIndication.la libVirt_RASD.la libVirt_HostSystem.la
+libVirt_VirtualSystemManagementService_la_DEPENDENCIES = libVirt_ComputerSystem.la
libVirt_ComputerSystemIndication.la libVirt_RASD.la libVirt_HostSystem.la
libVirt_DevicePool.la
libVirt_VirtualSystemManagementService_la_SOURCES =
Virt_VirtualSystemManagementService.c
-libVirt_VirtualSystemManagementService_la_LIBADD = -lVirt_ComputerSystem
-lVirt_ComputerSystemIndication -lVirt_RASD -lVirt_HostSystem
+libVirt_VirtualSystemManagementService_la_LIBADD = -lVirt_ComputerSystem
-lVirt_ComputerSystemIndication -lVirt_RASD -lVirt_HostSystem -lVirt_DevicePool
libVirt_VirtualSystemManagementCapabilities_la_DEPENDENCIES = libVirt_HostSystem.la
libVirt_VirtualSystemManagementCapabilities_la_SOURCES =
Virt_VirtualSystemManagementCapabilities.c
diff -r 3d143b851ee9 -r 01e5b1388ee7 src/Virt_VirtualSystemManagementService.c
--- a/src/Virt_VirtualSystemManagementService.c Wed Apr 09 14:29:08 2008 -0700
+++ b/src/Virt_VirtualSystemManagementService.c Wed Apr 09 14:31:20 2008 -0700
@@ -47,6 +47,7 @@
#include "Virt_ComputerSystemIndication.h"
#include "Virt_RASD.h"
#include "Virt_HostSystem.h"
+#include "Virt_DevicePool.h"
#include "svpc_types.h"
const static CMPIBroker *_BROKER;
@@ -202,8 +203,16 @@ static int xen_net_rasd_to_vdev(CMPIInst
static int xen_net_rasd_to_vdev(CMPIInstance *inst,
struct virt_device *dev)
{
+ const char *val = NULL;
+
free(dev->dev.net.type);
- dev->dev.net.type = strdup("bridge");
+ dev->dev.net.type = strdup("network");
+
+ if (cu_get_str_prop(inst, "PoolID", &val) != CMPI_RC_OK)
+ val = "NetworkPool/default";
+
+ free(dev->dev.net.source);
+ dev->dev.net.source = name_from_pool_id(val);
return 1;
}
@@ -211,11 +220,78 @@ static int kvm_net_rasd_to_vdev(CMPIInst
static int kvm_net_rasd_to_vdev(CMPIInstance *inst,
struct virt_device *dev)
{
+ const char *val = NULL;
+
free(dev->dev.net.type);
dev->dev.net.type = strdup("network");
+ if (cu_get_str_prop(inst, "PoolID", &val) != CMPI_RC_OK)
+ val = "NetworkPool/default";
+
free(dev->dev.net.source);
- dev->dev.net.source = strdup("default");
+ dev->dev.net.source = name_from_pool_id(val);
+
+ return 1;
+}
+
+static int net_rasd_to_vdev(CMPIInstance *inst,
+ struct virt_device *dev)
+{
+ const char *val = NULL;
+ CMPIObjectPath *op;
+
+ if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK)
+ val = "00:00:00:00:00:00";
+
+ free(dev->dev.net.mac);
+ dev->dev.net.mac = strdup(val);
+
+ op = CMGetObjectPath(inst, NULL);
+ if (op == NULL)
+ goto out;
+
+ if (STARTS_WITH(CLASSNAME(op), "Xen"))
+ xen_net_rasd_to_vdev(inst, dev);
+ else if (STARTS_WITH(CLASSNAME(op), "KVM"))
+ kvm_net_rasd_to_vdev(inst, dev);
+ else
+ CU_DEBUG("Unknown class type for net device: %s",
+ CLASSNAME(op));
+
+ out:
+ return 1;
+}
+
+static int disk_rasd_to_vdev(CMPIInstance *inst,
+ struct virt_device *dev)
+{
+ const char *val = NULL;
+
+ if (cu_get_str_prop(inst, "VirtualDevice", &val) != CMPI_RC_OK)
+ val = "hda";
+
+ free(dev->dev.disk.virtual_dev);
+ dev->dev.disk.virtual_dev = strdup(val);
+
+ if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK)
+ val = "/dev/null";
+
+ free(dev->dev.disk.source);
+ dev->dev.disk.source = strdup(val);
+ dev->dev.disk.disk_type = disk_type_from_file(val);
+
+ return 1;
+}
+
+static int mem_rasd_to_vdev(CMPIInstance *inst,
+ struct virt_device *dev)
+{
+ cu_get_u64_prop(inst, "VirtualQuantity", &dev->dev.mem.size);
+ cu_get_u64_prop(inst, "Reservation", &dev->dev.mem.size);
+ dev->dev.mem.maxsize = dev->dev.mem.size;
+ cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize);
+ dev->dev.mem.size <<= 10;
+ dev->dev.mem.maxsize <<= 10;
return 1;
}
@@ -224,11 +300,8 @@ static int rasd_to_vdev(CMPIInstance *in
struct virt_device *dev)
{
uint16_t type;
- const char *id = NULL;
- const char *val = NULL;
- char *name = NULL;
- char *devid = NULL;
CMPIObjectPath *op;
+ int ret = 0;
op = CMGetObjectPath(inst, NULL);
if (op == NULL)
@@ -239,50 +312,16 @@ static int rasd_to_vdev(CMPIInstance *in
dev->type = (int)type;
- if (cu_get_str_prop(inst, "InstanceID", &id) != CMPI_RC_OK)
- goto err;
-
- if (!parse_fq_devid(id, &name, &devid))
- goto err;
-
if (type == CIM_RES_TYPE_DISK) {
- free(dev->dev.disk.virtual_dev);
- dev->dev.disk.virtual_dev = devid;
-
- if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK)
- val = "/dev/null";
-
- free(dev->dev.disk.source);
- dev->dev.disk.source = strdup(val);
- dev->dev.disk.disk_type = disk_type_from_file(val);
+ ret = disk_rasd_to_vdev(inst, dev);
} else if (type == CIM_RES_TYPE_NET) {
- free(dev->dev.net.mac);
- dev->dev.net.mac = devid;
-
- if (STARTS_WITH(CLASSNAME(op), "Xen"))
- xen_net_rasd_to_vdev(inst, dev);
- else if (STARTS_WITH(CLASSNAME(op), "KVM"))
- kvm_net_rasd_to_vdev(inst, dev);
- else
- CU_DEBUG("Unknown class type for net device: %s",
- CLASSNAME(op));
-
+ ret = net_rasd_to_vdev(inst, dev);
} else if (type == CIM_RES_TYPE_MEM) {
- cu_get_u64_prop(inst, "VirtualQuantity",
&dev->dev.mem.size);
- cu_get_u64_prop(inst, "Reservation",
&dev->dev.mem.size);
- dev->dev.mem.maxsize = dev->dev.mem.size;
- cu_get_u64_prop(inst, "Limit", &dev->dev.mem.maxsize);
- dev->dev.mem.size <<= 10;
- dev->dev.mem.maxsize <<= 10;
- }
-
- free(name);
-
- return 1;
+ ret = mem_rasd_to_vdev(inst, dev);
+ }
+
+ return ret;
err:
- free(name);
- free(devid);
-
return 0;
}
5:34 p.m.
New subject: [PATCH 6 of 6] Make VSMS not parse the InstanceID of a RASD, but rather use the proper fields
const static CMPIBroker *_BROKER;
@@ -202,8 +203,16 @@ static int xen_net_rasd_to_vdev(CMPIInst
static int xen_net_rasd_to_vdev(CMPIInstance *inst,
struct virt_device *dev)
{
+ const char *val = NULL;
+
free(dev->dev.net.type);
- dev->dev.net.type = strdup("bridge");
+ dev->dev.net.type = strdup("network");
+
+ if (cu_get_str_prop(inst, "PoolID", &val) != CMPI_RC_OK)
+ val = "NetworkPool/default";
The default network pool isn't guaranteed to be present. In such a case,
the creation of a guest using "network" will fail.
Would it be possible to print a meaningful error message in this case?
We currently print "Failed to create domain" which isn't very descriptive.
Should we use the libvirt API to attempt to discover an available pool?
Also, why define val as "NetworkPool/default" just to strip out the
"NetworkPool/" piece later on? Is this for clarity / readability reasons?
+static int net_rasd_to_vdev(CMPIInstance *inst,
+ struct virt_device *dev)
+{
+ const char *val = NULL;
+ CMPIObjectPath *op;
+
+ if (cu_get_str_prop(inst, "Address", &val) != CMPI_RC_OK)
+ val = "00:00:00:00:00:00";
Why does this value differ from the random value assigned in the default
NetRASD?
I guess this brings up the question - should we be pulling these
defaults from the default RASDs? Or should we not assume defaults at
all and fail if an attribute we need isn't supplied?
The problem I see is that the profiles don't explicitly define what
attributes are required. It seems that the default RASDs define this
for us in some cases, but not all (see VirtualDevice and Address for
disk devices). Should these values be placed in the default RASDs as well?
Either way, I'm not sure it's meaningful to use a default value for
something such as the mac address. Maybe a distinction could be made -
for things like network pool, we can use a default (a statically defined
one as included above, or a discover a pool if available). For
something like a mac address, which really should be supplied by the
user, we can fail.
--
Kaitlin Rupert
IBM Linux Technology Center
kaitlin(a)linux.vnet.ibm.com
6:03 p.m.
New subject: [PATCH 6 of 6] Make VSMS not parse the InstanceID of a RASD, but rather use the proper fields
KR> The default network pool isn't guaranteed to be present. In such a
KR> case, the creation of a guest using "network" will fail.
Good point.
KR> Would it be possible to print a meaningful error message in this
KR> case? We currently print "Failed to create domain" which isn't
KR> very descriptive.
Yeah, we're pretty deep in the plumbing here, I'll see how I can
improve that.
KR> Should we use the libvirt API to attempt to discover an available
KR> pool?
We'd need to establish a connection to do that, which seems a little
hefty. I wonder what happens if we request a 'network' type without
specifying the network we want? Maybe it will take the first one...
KR> Also, why define val as "NetworkPool/default" just to strip out
KR> the "NetworkPool/" piece later on? Is this for clarity /
KR> readability reasons?
Yeah, it lets the rest of the code assume you've got a valid Pool
InstanceID in your PoolID, which is what would happen if you had
allocated it properly with AC. This makes it effective take the exact
same path for the default case, which I think is a reasonable
expectation.
KR> Why does this value differ from the random value assigned in the
KR> default NetRASD?
Well, because I was thinking that if you didn't specify a MAC and/or
you didn't go through the allocation step with AC. However, I'm not
sure this is a sane way to handle it, so it's good you brought it up.
KR> I guess this brings up the question - should we be pulling these
KR> defaults from the default RASDs? Or should we not assume defaults
KR> at all and fail if an attribute we need isn't supplied?
I tend to want to say that the DefineSystem() call is not an
allocation step. You either specify the resources you want, or that
you have allocated from AC, or it's an error. There are some fields
of a RASD that aren't required, of course, but there are some that
should be mandatory. In the network RASD, I think MAC would count as
mandatory.
If later, we wanted to have the ability to control the MAC allocation
per pool (say, to check to see if a randomly-generated MAC was already
in use in the pool), doing it automatically here would be a little
harder to reconcile, I think. My feeling is that failure to specify
something this fundamental in your domain config should be an error.
What do others think?
KR> The problem I see is that the profiles don't explicitly define
KR> what attributes are required. It seems that the default RASDs
KR> define this for us in some cases, but not all (see VirtualDevice
KR> and Address for disk devices). Should these values be placed in
KR> the default RASDs as well?
Well, I'm not sure that it makes sense to have a default path for the
default DiskRASD, although a default VirtualDevice of 'xvda' for xen
and 'hda' for KVM seems sane to me.
KR> Either way, I'm not sure it's meaningful to use a default value
KR> for something such as the mac address. Maybe a distinction could
KR> be made for things like network pool, we can use a default (a
KR> statically defined one as included above, or a discover a pool if
KR> available). For something like a mac address, which really should
KR> be supplied by the user, we can fail.
Yeah, I think that if they specify the MAC and don't specify a pool,
it makes sense to use the default (or first-found pool). I'll check
into the libvirt behavior mentioned above and post another round for
comments.
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
6062
days inactive
6063
days old
8 comments
2 participants
participants (2)
-
Dan Smith -
Kaitlin Rupert