[PATCH 0 of 6] Fix up handling of NetRASD-related define path
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 :)
# HG changeset patch # User Dan Smith <danms@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@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 };
# HG changeset patch # User Dan Smith <danms@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@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;
# HG changeset patch # User Dan Smith <danms@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@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";
# HG changeset patch # User Dan Smith <danms@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@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";
# HG changeset patch # User Dan Smith <danms@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@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 * *
# HG changeset patch # User Dan Smith <danms@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@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; }
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@linux.vnet.ibm.com
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@us.ibm.com
participants (2)
-
Dan Smith -
Kaitlin Rupert