[PATCH 0 of 6] SettingsDefineCapabilities, all of it
So this is a huge set, and I'm sorry for dumping it all at once, but I kept bouncing between the various patches as I worked on it, so sending any one out earlier probably would have backfired. This brings SettingsDefineCapabilities to a working state, as well as does all the background work. Clients should now be able to follow the chain of associations all the way from a HostSystem instance to a series of RASD instances that tell them the min, max, increment (read: granulatiry), and default values for NetworkPort, Disk, Memory, and Processor. Once this set is processed and approved I have some plans to try and make things a little more compact, as there is a reasonable amount of duplication in some places, and a couple of cases could use some polish, but with this set they should all be working, so it was about time to get it out for review.
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557266 18000 # Node ID fd6deb234772b44fe549a3513a47115a01e20f7d # Parent bd1b1067d106ecc1546c3b2436a43f59f8eaba15 Turns out that the sdc_rasd_prop functions might need to report status or see the passed in reference. This updates the typedef and the memory functions, since those are already in the tree. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r bd1b1067d106 -r fd6deb234772 src/Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 10:43:48 2007 -0800 +++ b/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:27:46 2007 -0500 @@ -96,84 +96,96 @@ static bool free_rasd_prop_list(struct s return true; } -static struct sdc_rasd_prop *mem_max(void) -{ - bool ret; - struct sdc_rasd_prop *rasd; +static struct sdc_rasd_prop *mem_max(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + struct sdc_rasd_prop *rasd = NULL; uint64_t max_vq = MAX_MEM; - struct sdc_rasd_prop max[] = { + struct sdc_rasd_prop tmp[] = { {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, {"AllocationUnits", (CMPIValue *)"MegaBytes", CMPI_chars}, {"VirtualQuantity", (CMPIValue *)&max_vq, CMPI_uint64}, PROP_END }; - ret = dup_rasd_prop_list(max, &rasd); - if (ret) - return rasd; - else - return NULL; -} - -static struct sdc_rasd_prop *mem_min(void) -{ - bool ret; - struct sdc_rasd_prop *rasd; + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + +static struct sdc_rasd_prop *mem_min(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + struct sdc_rasd_prop *rasd = NULL; uint64_t min_vq = 64; - struct sdc_rasd_prop min[] = { + struct sdc_rasd_prop tmp[] = { {"InstanceID", (CMPIValue *)"Minimum", CMPI_chars}, {"AllocationUnits", (CMPIValue *)"MegaBytes", CMPI_chars}, {"VirtualQuantity", (CMPIValue *)&min_vq, CMPI_uint64}, PROP_END }; - ret = dup_rasd_prop_list(min, &rasd); - if (ret) - return rasd; - else - return NULL; -} - -static struct sdc_rasd_prop *mem_def(void) -{ - bool ret; - struct sdc_rasd_prop *rasd; + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + +static struct sdc_rasd_prop *mem_def(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + struct sdc_rasd_prop *rasd = NULL; uint64_t def_vq = 256; - struct sdc_rasd_prop def[] = { + struct sdc_rasd_prop tmp[] = { {"InstanceID", (CMPIValue *)"Default", CMPI_chars}, {"AllocationUnits", (CMPIValue *)"MegaBytes", CMPI_chars}, {"VirtualQuantity", (CMPIValue *)&def_vq, CMPI_uint64}, PROP_END }; - ret = dup_rasd_prop_list(def, &rasd); - if (ret) - return rasd; - else - return NULL; -} - -static struct sdc_rasd_prop *mem_inc(void) -{ - bool ret; - struct sdc_rasd_prop *rasd; + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + +static struct sdc_rasd_prop *mem_inc(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + struct sdc_rasd_prop *rasd = NULL; uint64_t inc_vq = 1; - struct sdc_rasd_prop inc[] = { + struct sdc_rasd_prop tmp[] = { {"InstanceID", (CMPIValue *)"Increment", CMPI_chars}, {"AllocationUnits", (CMPIValue *)"MegaBytes", CMPI_chars}, {"VirtualQuantity", (CMPIValue *)&inc_vq, CMPI_uint64}, PROP_END }; - ret = dup_rasd_prop_list(inc, &rasd); - if (ret) - return rasd; - else - return NULL; + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; } static struct sdc_rasd mem = { @@ -190,17 +202,17 @@ static struct sdc_rasd *sdc_rasd_list[] }; static CMPIInstance *sdc_rasd_inst(const CMPIBroker *broker, + CMPIStatus *s, const CMPIObjectPath *ref, struct sdc_rasd *rasd, sdc_rasd_type type) { - CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *inst = NULL; struct sdc_rasd_prop *prop_list; int i; char *inst_id; uint16_t resource_type; - /* Defaults for the following three values are from + /* Defaults for the following are from CIM_SettingsDefineCapabilities.mof. */ uint16_t policy = SDC_POLICY_INDEPENDENT; uint16_t role = SDC_ROLE_SUPPORTED; @@ -209,43 +221,40 @@ static CMPIInstance *sdc_rasd_inst(const case SDC_RASD_MIN: if (rasd->min == NULL) goto out; - prop_list = rasd->min(); + prop_list = rasd->min(ref, s); inst_id = "Minimum"; range = SDC_RANGE_MIN; break; case SDC_RASD_MAX: if (rasd->max == NULL) goto out; - prop_list = rasd->max(); + prop_list = rasd->max(ref, s); inst_id = "Maximum"; range = SDC_RANGE_MAX; break; case SDC_RASD_INC: if (rasd->inc == NULL) goto out; - prop_list = rasd->inc(); + prop_list = rasd->inc(ref, s); inst_id = "Increment"; range = SDC_RANGE_INC; break; case SDC_RASD_DEF: if (rasd->def == NULL) goto out; - prop_list = rasd->def(); + prop_list = rasd->def(ref, s); inst_id = "Default"; role = SDC_ROLE_DEFAULT; range = SDC_RANGE_POINT; break; default: - CMSetStatusWithChars(broker, &s, CMPI_RC_ERR_FAILED, + CMSetStatusWithChars(broker, s, CMPI_RC_ERR_FAILED, "Unsupported sdc_rasd type."); goto out; } - if (prop_list == NULL) { - CMSetStatusWithChars(broker, &s, CMPI_RC_ERR_FAILED, - "Failed to get prop_list."); + if (s->rc != CMPI_RC_OK) goto out; - } inst = get_typed_instance(broker, "ResourceAllocationSettingData", @@ -291,7 +300,11 @@ static CMPIStatus sdc_rasds_for_type(con if (rasd) { for (i = SDC_RASD_MIN; i <= SDC_RASD_INC; i++) { - inst = sdc_rasd_inst(_BROKER, ref, rasd, i); + inst = sdc_rasd_inst(_BROKER, &s, ref, rasd, i); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Problem getting inst."); + goto out; + } CU_DEBUG("Got inst.\n"); if (inst != NULL) { inst_list_add(list, inst); @@ -307,6 +320,7 @@ static CMPIStatus sdc_rasds_for_type(con "Unsupported device type."); } + out: return s; } diff -r bd1b1067d106 -r fd6deb234772 src/Virt_SettingsDefineCapabilities.h --- a/src/Virt_SettingsDefineCapabilities.h Thu Nov 08 10:43:48 2007 -0800 +++ b/src/Virt_SettingsDefineCapabilities.h Thu Nov 08 16:27:46 2007 -0500 @@ -50,7 +50,8 @@ struct sdc_rasd_prop { CMPIType type; }; -typedef struct sdc_rasd_prop *(*rasd_prop_func_t)(void); +typedef struct sdc_rasd_prop *(*rasd_prop_func_t)(const CMPIObjectPath *ref, + CMPIStatus *s); struct sdc_rasd { uint16_t resource_type;
JG> # HG changeset patch JG> # User Jay Gagnon <grendel@linux.vnet.ibm.com> JG> # Date 1194557266 18000 JG> # Node ID fd6deb234772b44fe549a3513a47115a01e20f7d JG> # Parent bd1b1067d106ecc1546c3b2436a43f59f8eaba15 JG> Turns out that the sdc_rasd_prop functions might need to report status or see the passed in reference. This updates the typedef and the memory functions, since those are already in the tree. This one looks pretty straightforward. It looks like we could apply this one independently if we wanted to, right? A rather self-contained change, considering the size of the rest of the set. JG> + if (!ret) { JG> + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, JG> + "Could not copy RASD."); JG> + } This is a total nit, so forgive my OCD, but I think that everywhere else, the style of a cu_statusf() call is: cu_statusf(broker, status, <newline> CMPI_RC_ERR_FOO, <newline> "Error message", [opt if short, else newline], [opt]); Could I convince you to change these? :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> # HG changeset patch JG> # User Jay Gagnon <grendel@linux.vnet.ibm.com> JG> # Date 1194557266 18000 JG> # Node ID fd6deb234772b44fe549a3513a47115a01e20f7d JG> # Parent bd1b1067d106ecc1546c3b2436a43f59f8eaba15 JG> Turns out that the sdc_rasd_prop functions might need to report status or see the passed in reference. This updates the typedef and the memory functions, since those are already in the tree.
This one looks pretty straightforward. It looks like we could apply this one independently if we wanted to, right? A rather self-contained change, considering the size of the rest of the set.
This sets up the new function header for all the callbacks. I think it can be applied separately in that it can be applied before the other ones, but if we are going to put those in then this one will have to be there. Did that answer the question?
JG> + if (!ret) { JG> + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, JG> + "Could not copy RASD."); JG> + }
This is a total nit, so forgive my OCD, but I think that everywhere else, the style of a cu_statusf() call is:
cu_statusf(broker, status, <newline> CMPI_RC_ERR_FOO, <newline> "Error message", [opt if short, else newline], [opt]);
Could I convince you to change these? :)
Yea, no prob. -- -Jay
Jay Gagnon wrote:
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557266 18000 # Node ID fd6deb234772b44fe549a3513a47115a01e20f7d # Parent bd1b1067d106ecc1546c3b2436a43f59f8eaba15 Turns out that the sdc_rasd_prop functions might need to report status or see the passed in reference. This updates the typedef and the memory functions, since those are already in the tree. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com>
diff -r bd1b1067d106 -r fd6deb234772 src/Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 10:43:48 2007 -0800 +++ b/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:27:46 2007 -0500 @@ -96,84 +96,96 @@ static bool free_rasd_prop_list(struct s return true; }
-static struct sdc_rasd_prop *mem_max(void) -{ - bool ret; - struct sdc_rasd_prop *rasd; +static struct sdc_rasd_prop *mem_max(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + struct sdc_rasd_prop *rasd = NULL; uint64_t max_vq = MAX_MEM;
- struct sdc_rasd_prop max[] = { + struct sdc_rasd_prop tmp[] = { {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, {"AllocationUnits", (CMPIValue *)"MegaBytes", CMPI_chars}, {"VirtualQuantity", (CMPIValue *)&max_vq, CMPI_uint64}, PROP_END };
- ret = dup_rasd_prop_list(max, &rasd); - if (ret) - return rasd; - else - return NULL; -} - -static struct sdc_rasd_prop *mem_min(void) -{ - bool ret; - struct sdc_rasd_prop *rasd; + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +}
Why is the ref param needed here? Is that something you'll be adding later? -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Kaitlin Rupert wrote:
Jay Gagnon wrote:
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557266 18000 # Node ID fd6deb234772b44fe549a3513a47115a01e20f7d # Parent bd1b1067d106ecc1546c3b2436a43f59f8eaba15 Turns out that the sdc_rasd_prop functions might need to report status or see the passed in reference. This updates the typedef and the memory functions, since those are already in the tree. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com>
Why is the ref param needed here? Is that something you'll be adding later?
For the most part, the ref param is unneeded, but some of the callbacks (disk_max for example) need to look at the ref to determine which Pool they are being asked about. All of the callbacks need to have the same header because the sdc_rasd struct stores function pointers for the callbacks and I really didn't want to do it as the always foreboding (void *). -- -Jay
JG> All of the callbacks need to have the same header because the JG> sdc_rasd struct stores function pointers for the callbacks Right, you're doing the correct thing here, IMHO. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> All of the callbacks need to have the same header because the JG> sdc_rasd struct stores function pointers for the callbacks
Right, you're doing the correct thing here, IMHO.
Yep, agreed. I didn't see ref used in any of the functions in the first patch, so I was confused. But I see why it's useful now. =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557273 18000 # Node ID 543a0790d8615551153950de8f2f2fe3de107cf3 # Parent fd6deb234772b44fe549a3513a47115a01e20f7d Need to reorder the .la lines in makefile since SettingsDefineCapabilities now depends on DevicePool. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r fd6deb234772 -r 543a0790d861 src/Makefile.am --- a/src/Makefile.am Thu Nov 08 16:27:46 2007 -0500 +++ b/src/Makefile.am Thu Nov 08 16:27:53 2007 -0500 @@ -33,12 +33,12 @@ provider_LTLIBRARIES = libVirt_ComputerS libVirt_VirtualSystemManagementCapabilities.la \ libVirt_EnabledLogicalElementCapabilities.la \ libVirt_AllocationCapabilities.la \ - libVirt_SettingsDefineCapabilities.la \ libVirt_VSSD.la \ libVirt_HostedDependency.la \ libVirt_RegisteredProfile.la \ libVirt_ElementConformsToProfile.la \ libVirt_DevicePool.la \ + libVirt_SettingsDefineCapabilities.la \ libVirt_HostedResourcePool.la \ libVirt_ElementCapabilities.la \ libVirt_ResourcePoolConfigurationService.la \
JG> # HG changeset patch JG> # User Jay Gagnon <grendel@linux.vnet.ibm.com> JG> # Date 1194557273 18000 JG> # Node ID 543a0790d8615551153950de8f2f2fe3de107cf3 JG> # Parent fd6deb234772b44fe549a3513a47115a01e20f7d JG> Need to reorder the .la lines in makefile since SettingsDefineCapabilities now depends on DevicePool. +1, obviously :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557358 18000 # Node ID 94308147bed1693443d0741de6a30c5b0f77b0f1 # Parent 543a0790d8615551153950de8f2f2fe3de107cf3 Add Disk support to SettingsDefineCapabilities. Still lacks a bit of polish (i.e. lack of warning when minimum or default is higher than maximum), but is functional. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 543a0790d861 -r 94308147bed1 src/Makefile.am --- a/src/Makefile.am Thu Nov 08 16:27:53 2007 -0500 +++ b/src/Makefile.am Thu Nov 08 16:29:18 2007 -0500 @@ -85,7 +85,7 @@ libVirt_AllocationCapabilities_la_LIBADD libVirt_AllocationCapabilities_la_LIBADD = -lVirt_RASD libVirt_SettingsDefineCapabilities_la_SOURCES = Virt_SettingsDefineCapabilities.c -libVirt_SettingsDefineCapabilities_la_LIBADD = -lVirt_RASD +libVirt_SettingsDefineCapabilities_la_LIBADD = -lVirt_RASD -lVirt_DevicePool libVirt_RegisteredProfile_la_SOURCES = Virt_RegisteredProfile.c diff -r 543a0790d861 -r 94308147bed1 src/Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:27:53 2007 -0500 +++ b/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:29:18 2007 -0500 @@ -23,6 +23,8 @@ #include <stdlib.h> #include <stdio.h> #include <stdbool.h> +#include <sys/vfs.h> +#include <errno.h> #include "config.h" @@ -37,8 +39,13 @@ #include "svpc_types.h" #include "Virt_SettingsDefineCapabilities.h" +#include "Virt_DevicePool.h" const static CMPIBroker *_BROKER; + +/* These are used in more than one place so they are defined here. */ +#define SDC_DISK_MIN 2000 +#define SDC_DISK_DEF 5000 static bool rasd_prop_copy_value(struct sdc_rasd_prop src, struct sdc_rasd_prop *dest) @@ -188,6 +195,137 @@ static struct sdc_rasd_prop *mem_inc(con return rasd; } +static struct sdc_rasd_prop *disk_min(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t disk_size = SDC_DISK_MIN; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Minimum", CMPI_chars}, + {"AllocationQuantity", (CMPIValue *)"MegaBytes", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&disk_size, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + +static struct sdc_rasd_prop *disk_max(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + char *inst_id; + CMPIrc prop_ret; + uint16_t free_space; + uint64_t free_64; + virConnectPtr conn; + CMPIInstance *pool_inst; + struct sdc_rasd_prop *rasd = NULL; + + inst_id = cu_get_str_path(ref, "InstanceID"); + if (inst_id == NULL) { + CMSetStatusWithChars(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not get InstanceID."); + goto out; + } + + conn = lv_connect(_BROKER, s); + if (s->rc != CMPI_RC_OK) { + CMSetStatusWithChars(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not get connection."); + goto out; + } + + /* Getting the relevant resource pool directly finds the free space + for us. It is in the Capacity field. */ + pool_inst = get_pool_by_id(_BROKER, conn, inst_id, NAMESPACE(ref)); + if (pool_inst == NULL) { + CMSetStatusWithChars(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not get pool instance."); + goto out; + } + + prop_ret = cu_get_u64_prop(pool_inst, "Capacity", &free_64); + if (prop_ret != CMPI_RC_OK) { + CMSetStatusWithChars(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not get capacity from instance."); + goto out; + } + CU_DEBUG("Got capacity from pool_inst: %lld", free_64); + + free_space = (uint16_t)free_64; + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, + {"AllocationQuantity", (CMPIValue *)"MegaBytes", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&free_space, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + out: + free(inst_id); + return rasd; +} + +static struct sdc_rasd_prop *disk_def(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t disk_size = SDC_DISK_DEF; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Default", CMPI_chars}, + {"AllocationQuantity", (CMPIValue *)"MegaBytes", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&disk_size, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + +static struct sdc_rasd_prop *disk_inc(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t disk_size = 250; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Increment", CMPI_chars}, + {"AllocationQuantity", (CMPIValue *)"MegaBytes", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&disk_size, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + static struct sdc_rasd mem = { .resource_type = CIM_RASD_TYPE_MEM, .min = mem_min, @@ -196,8 +334,17 @@ static struct sdc_rasd mem = { .inc = mem_inc }; +static struct sdc_rasd disk = { + .resource_type = CIM_RASD_TYPE_DISK, + .min = disk_min, + .max = disk_max, + .def = disk_def, + .inc = disk_inc +}; + static struct sdc_rasd *sdc_rasd_list[] = { &mem, + &disk, NULL };
JG> # HG changeset patch JG> # User Jay Gagnon <grendel@linux.vnet.ibm.com> JG> # Date 1194557358 18000 JG> # Node ID 94308147bed1693443d0741de6a30c5b0f77b0f1 JG> # Parent 543a0790d8615551153950de8f2f2fe3de107cf3 JG> Add Disk support to SettingsDefineCapabilities. Still lacks a bit of polish (i.e. lack of warning when minimum or default is higher than maximum), but is functional. I think this looks okay, but: JG> +#define SDC_DISK_MIN 2000 JG> +#define SDC_DISK_DEF 5000 Why not make the increment amount also parameterized as such? Seems like a valid thing to want to change. Unlikely, but valid :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> # HG changeset patch JG> # User Jay Gagnon <grendel@linux.vnet.ibm.com> JG> # Date 1194557358 18000 JG> # Node ID 94308147bed1693443d0741de6a30c5b0f77b0f1 JG> # Parent 543a0790d8615551153950de8f2f2fe3de107cf3 JG> Add Disk support to SettingsDefineCapabilities. Still lacks a bit of polish (i.e. lack of warning when minimum or default is higher than maximum), but is functional.
I think this looks okay, but:
JG> +#define SDC_DISK_MIN 2000 JG> +#define SDC_DISK_DEF 5000
Why not make the increment amount also parameterized as such? Seems like a valid thing to want to change. Unlikely, but valid :)
Sure, that's fine. With those two being defined as constants for disk it's logical to define the last one as well, for organizational purposes. That said, the next logical question could be, "Why aren't the other numbers all defines as well?" My answer to that is, as a habit I tend to only make things that are (or in this case, will be) used in more than one place, defines. The way I see it, if I only use a number once, having it as a define doesn't get me very far, because the define really just creates a level of "indirection" for whomever decides the value needs to be changed. As soon as the number is used twice, you of course need a define so that nobody has to root out all instances of the value, but for one-time-use-only values I don't generally bother with defines. This is a pretty good case for an exception to the rule, though, since the lack of an INC define could be misleading. -- -Jay
JG> The way I see it, if I only use a number once, having it as a JG> define doesn't get me very far, because the define really just JG> creates a level of "indirection" for whomever decides the value JG> needs to be changed. As soon as the number is used twice, you of JG> course need a define so that nobody has to root out all instances JG> of the value, but for one-time-use-only values I don't generally JG> bother with defines. Well, even if something is only used once, it makes it easier to change something into a configure option if it already has that layer of indirection in place. It also means that someone can easily open the file, see that the limits are all defined at the top, and not bother to understand the rest of the function pointer indirection (which would be harder to follow without context than the #defines anyway). The only things that I think make sense to leave buried are things like increment for NICs, since it's very clear that adding one NIC will always be the desired granularity, which can't be said for disk or memory, IMHO. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557377 18000 # Node ID 0ed8747c1dfbf14ff7f6e23ff792a35852294fa6 # Parent 94308147bed1693443d0741de6a30c5b0f77b0f1 Add NetworkPort functionality to SettingsDefineCapabilities. All static, max may become dynamic. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 94308147bed1 -r 0ed8747c1dfb src/Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:29:18 2007 -0500 +++ b/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:29:37 2007 -0500 @@ -195,6 +195,85 @@ static struct sdc_rasd_prop *mem_inc(con return rasd; } +static struct sdc_rasd_prop *net_min(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t num_nics = 0; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Minimum", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (ret) + return rasd; + else + return NULL; +} + +static struct sdc_rasd_prop *net_max(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t num_nics = 6; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (ret) + return rasd; + else + return NULL; +} +static struct sdc_rasd_prop *net_def(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t num_nics = 1; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Default", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (ret) + return rasd; + else + return NULL; +} + +static struct sdc_rasd_prop *net_inc(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t num_nics = 1; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Increment", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (ret) + return rasd; + else + return NULL; +} + static struct sdc_rasd_prop *disk_min(const CMPIObjectPath *ref, CMPIStatus *s) { @@ -334,6 +413,14 @@ static struct sdc_rasd mem = { .inc = mem_inc }; +static struct sdc_rasd network = { + .resource_type = CIM_RASD_TYPE_NET, + .min = net_min, + .max = net_max, + .def = net_def, + .inc = net_inc +}; + static struct sdc_rasd disk = { .resource_type = CIM_RASD_TYPE_DISK, .min = disk_min, @@ -344,6 +431,7 @@ static struct sdc_rasd disk = { static struct sdc_rasd *sdc_rasd_list[] = { &mem, + &network, &disk, NULL };
JG> + uint16_t num_nics = 6; JG> + struct sdc_rasd_prop *rasd = NULL; JG> + JG> + struct sdc_rasd_prop tmp[] = { JG> + {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, JG> + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, JG> + PROP_END JG> + }; The maximum NIC count is probably different for KVM and Xen, and I imagine this function should be intelligent in the future. Could we at least pull the '6' out to a constant like XEN_MAX_VIF or something? JG> +static struct sdc_rasd_prop *net_def(const CMPIObjectPath *ref, JG> + CMPIStatus *s) JG> +{ JG> + bool ret; JG> + uint16_t num_nics = 1; JG> + struct sdc_rasd_prop *rasd = NULL; JG> + JG> + struct sdc_rasd_prop tmp[] = { JG> + {"InstanceID", (CMPIValue *)"Default", CMPI_chars}, JG> + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, JG> + PROP_END JG> + }; JG> + JG> + ret = dup_rasd_prop_list(tmp, &rasd); JG> + if (ret) JG> + return rasd; JG> + else JG> + return NULL; JG> +} JG> + JG> +static struct sdc_rasd_prop *net_inc(const CMPIObjectPath *ref, JG> + CMPIStatus *s) JG> +{ JG> + bool ret; JG> + uint16_t num_nics = 1; JG> + struct sdc_rasd_prop *rasd = NULL; JG> + JG> + struct sdc_rasd_prop tmp[] = { JG> + {"InstanceID", (CMPIValue *)"Increment", CMPI_chars}, JG> + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, JG> + PROP_END JG> + }; JG> + JG> + ret = dup_rasd_prop_list(tmp, &rasd); JG> + if (ret) JG> + return rasd; JG> + else JG> + return NULL; JG> +} I think "default" and "increment" in the network case are probably safe to leave hardcoded to '1' and buried :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
On Thu, Nov 08, 2007 at 03:58:26PM -0800, Dan Smith wrote:
JG> + uint16_t num_nics = 6; JG> + struct sdc_rasd_prop *rasd = NULL; JG> + JG> + struct sdc_rasd_prop tmp[] = { JG> + {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, JG> + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, JG> + PROP_END JG> + };
The maximum NIC count is probably different for KVM and Xen, and I imagine this function should be intelligent in the future. Could we at least pull the '6' out to a constant like XEN_MAX_VIF or something?
As that bug points out, 6 sounds dangerously low for a virtual interface: https://bugzilla.redhat.com/show_bug.cgi?id=273421 Any chance to also increase this at least up to 32 ? Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
DV> Any chance to also increase this at least up to 32 ? Last I checked, Xen had a hard limit due to a ringbuffer sizing issue or something; has this changed? We were planning to try to expose this limit (adjusted per platform) to the client. Note that this is an advertised maximum that is not strictly enforced. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> + uint16_t num_nics = 6; JG> + struct sdc_rasd_prop *rasd = NULL; JG> + JG> + struct sdc_rasd_prop tmp[] = { JG> + {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, JG> + {"VirtualQuantity", (CMPIValue *)&num_nics, CMPI_uint16}, JG> + PROP_END JG> + };
The maximum NIC count is probably different for KVM and Xen, and I imagine this function should be intelligent in the future. Could we at least pull the '6' out to a constant like XEN_MAX_VIF or something?
I agree that this should probably be made intelligent in the future, but as I said with regards to the disk patch, I don't necessarily see the point to making this a defined constant unless we need to use it elsewhere or we actually add a hardcoded value for KVM. As it is right now, the value is only ever assigned in that initialization line and only ever read four lines later, and its use seems clear enough that a named constant won't be adding any clarity. Not a huge deal, and, "Yea, that's a good point. Make it a define," is a fine response here, but that's my opinion on the matter. -- -Jay
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557443 18000 # Node ID 2d09468e2de7ed9194907bfff1efbb16ba40202d # Parent 0ed8747c1dfbf14ff7f6e23ff792a35852294fa6 Add processor support to SettingsDefineCapabilities. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 0ed8747c1dfb -r 2d09468e2de7 src/Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:29:37 2007 -0500 +++ b/src/Virt_SettingsDefineCapabilities.c Thu Nov 08 16:30:43 2007 -0500 @@ -26,6 +26,8 @@ #include <sys/vfs.h> #include <errno.h> +#include <libvirt.h> + #include "config.h" #include "cmpidt.h" @@ -195,6 +197,121 @@ static struct sdc_rasd_prop *mem_inc(con return rasd; } +static struct sdc_rasd_prop *proc_min(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t num_procs = 1; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Minimum", CMPI_chars}, + {"AllocationUnits", (CMPIValue *)"Processors", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_procs, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + +static struct sdc_rasd_prop *proc_max(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + int cpu_num; + FILE *cpuinfo; + size_t len = 0; + char *line = NULL; + uint16_t num_procs = 0; + struct sdc_rasd_prop *rasd = NULL; + + CU_DEBUG("In proc_max()"); + + cpuinfo = fopen("/proc/cpuinfo", "r"); + if (cpuinfo == NULL) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy open cpuinfo."); + goto out; + } + + while (getline(&line, &len, cpuinfo) > 0) { + if (sscanf(line, "processor : %d", &cpu_num) == 1) { + CU_DEBUG("Line matched. num_procs++"); + num_procs++; + } + } + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Maximum", CMPI_chars}, + {"AllocationUnits", (CMPIValue *)"Processors", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_procs, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + out: + free(line); + fclose(cpuinfo); + return rasd; +} + +static struct sdc_rasd_prop *proc_def(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t num_procs = 1; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Default", CMPI_chars}, + {"AllocationUnits", (CMPIValue *)"Processors", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_procs, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + +static struct sdc_rasd_prop *proc_inc(const CMPIObjectPath *ref, + CMPIStatus *s) +{ + bool ret; + uint16_t num_procs = 1; + struct sdc_rasd_prop *rasd = NULL; + + struct sdc_rasd_prop tmp[] = { + {"InstanceID", (CMPIValue *)"Increment", CMPI_chars}, + {"AllocationUnits", (CMPIValue *)"Processors", CMPI_chars}, + {"VirtualQuantity", (CMPIValue *)&num_procs, CMPI_uint16}, + PROP_END + }; + + ret = dup_rasd_prop_list(tmp, &rasd); + if (!ret) { + cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, + "Could not copy RASD."); + } + + return rasd; +} + static struct sdc_rasd_prop *net_min(const CMPIObjectPath *ref, CMPIStatus *s) { @@ -413,6 +530,14 @@ static struct sdc_rasd mem = { .inc = mem_inc }; +static struct sdc_rasd processor = { + .resource_type = CIM_RASD_TYPE_PROC, + .min = proc_min, + .max = proc_max, + .def = proc_def, + .inc = proc_inc +}; + static struct sdc_rasd network = { .resource_type = CIM_RASD_TYPE_NET, .min = net_min, @@ -431,6 +556,7 @@ static struct sdc_rasd disk = { static struct sdc_rasd *sdc_rasd_list[] = { &mem, + &processor, &network, &disk, NULL
JG> + cpuinfo = fopen("/proc/cpuinfo", "r"); This should come from virNodeInfo. Also, it would be good to go ahead and split out the "how many vcpus is the max for xen?" behavior into a separate function, because it's possible that other platforms will be able to overcommit vcpus. This might be a good place to start making it clear that the behavior is platform-dependent. I think it's okay to always take the Xen default for the time being, but while you're reworking this, you might as well split it out too. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
JG> + cpuinfo = fopen("/proc/cpuinfo", "r");
This should come from virNodeInfo. Okay. Only question before I do that is do we have virNodeInfo in all
Dan Smith wrote: the versions of libvirt we support? The only reason I had to write this myself at all is that the libvirt function that determines the maximum number of vcpus came in too late for us.
Also, it would be good to go ahead and split out the "how many vcpus is the max for xen?" behavior into a separate function, because it's possible that other platforms will be able to overcommit vcpus. This might be a good place to start making it clear that the behavior is platform-dependent. I think it's okay to always take the Xen default for the time being, but while you're reworking this, you might as well split it out too.
So for now proc_max will basically just call xen_proc_max but in the future we might need to check the ref and add another function? If that's what you meant, that's fine. -- -Jay
Jay Gagnon wrote:
Dan Smith wrote:
JG> + cpuinfo = fopen("/proc/cpuinfo", "r");
This should come from virNodeInfo.
Okay. Only question before I do that is do we have virNodeInfo in all the versions of libvirt we support? The only reason I had to write this myself at all is that the libvirt function that determines the maximum number of vcpus came in too late for us.
Looks like this question got lost in the shuffle. What version of libvirt are we actually writing against? In other words, of the versions shipped with the various distributions we're supporting, which one is the oldest? I want to make sure I don't try to use something that is too new. -- -Jay
JG> What version of libvirt are we actually writing against? In other JG> words, of the versions shipped with the various distributions JG> we're supporting, which one is the oldest? I want to make sure I JG> don't try to use something that is too new. I think that for right now, we should aim at the latest libvirt release. If someone needs to run libvirt-cim on an older stack, we can evaluate and address issues with a specific version in mind. That said, I'm not sure what version this node info call was added in, but it seems rather fundamental, so I would hope it's been around for a while :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
On Mon, Nov 12, 2007 at 10:26:54AM -0500, Jay Gagnon wrote:
Jay Gagnon wrote:
Dan Smith wrote:
JG> + cpuinfo = fopen("/proc/cpuinfo", "r");
This should come from virNodeInfo.
Okay. Only question before I do that is do we have virNodeInfo in all the versions of libvirt we support? The only reason I had to write this myself at all is that the libvirt function that determines the maximum number of vcpus came in too late for us.
Looks like this question got lost in the shuffle. What version of libvirt are we actually writing against? In other words, of the versions shipped with the various distributions we're supporting, which one is the oldest? I want to make sure I don't try to use something that is too new.
See virNodeGetInfo in the table http://libvirt.org/hvsupport.html it's there since 0.1.0 (at least for Xen), so no problem, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
On Mon, Nov 12, 2007 at 10:26:54AM -0500, Jay Gagnon wrote:
Jay Gagnon wrote:
Dan Smith wrote:
JG> + cpuinfo = fopen("/proc/cpuinfo", "r");
This should come from virNodeInfo.
Okay. Only question before I do that is do we have virNodeInfo in all the versions of libvirt we support? The only reason I had to write this myself at all is that the libvirt function that determines the maximum number of vcpus came in too late for us.
Looks like this question got lost in the shuffle. What version of libvirt are we actually writing against? In other words, of the versions shipped with the various distributions we're supporting, which one is the oldest? I want to make sure I don't try to use something that is too new.
See virNodeGetInfo in the table http://libvirt.org/hvsupport.html it's there since 0.1.0 (at least for Xen), so no problem,
Daniel
Heh, yea looks like we should be all right. I hadn't seen that table before, definitely good to know that exists. -- -Jay
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557532 18000 # Node ID cf0a2938ba0b9d2e7af8a5790d0e56cc7564df73 # Parent 2d09468e2de7ed9194907bfff1efbb16ba40202d Add another relationship to ElementCapabilities, the ResourcePool-AllocationCapabilities connection. Only supports the pool_to_alloc direction, because it is the only one that's terribly interesting, but the other direction will need to be added at some point. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 2d09468e2de7 -r cf0a2938ba0b src/Virt_ElementCapabilities.c --- a/src/Virt_ElementCapabilities.c Thu Nov 08 16:30:43 2007 -0500 +++ b/src/Virt_ElementCapabilities.c Thu Nov 08 16:32:12 2007 -0500 @@ -179,6 +179,50 @@ static CMPIStatus cap_to_cs(const CMPIOb return s; } +static CMPIStatus alloc_to_pool(const CMPIObjectPath *ref, + struct std_assoc_info *info, + struct inst_list *list) +{ + /* Pool to alloc is more important. That will be done first. */ + return (CMPIStatus){CMPI_RC_OK, NULL}; +} + +static CMPIStatus pool_to_alloc(const CMPIObjectPath *ref, + struct std_assoc_info *info, + struct inst_list *list) +{ + int ret; + char *inst_id; + uint16_t type; + CMPIInstance *inst = NULL; + CMPIStatus s = {CMPI_RC_OK}; + + inst_id = cu_get_str_path(ref, "InstanceID"); + if (inst_id == NULL) { + CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Could not get InstanceID."); + goto out; + } + + inst = get_typed_instance(_BROKER, "AllocationCapabilities", + NAMESPACE(ref)); + CMSetProperty(inst, "InstanceID", inst_id, CMPI_chars); + + ret = cu_get_u16_path(ref, "ResourceType", &type); + if (ret != 1) { + CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Could not get ResourceType."); + goto out; + } + CMSetProperty(inst, "ResourceType", &type, CMPI_uint16); + + inst_list_add(list, inst); + + out: + free(inst_id); + + return s; +} static CMPIInstance *make_ref(const CMPIObjectPath *ref, const CMPIInstance *inst, struct std_assoc_info *info, @@ -273,6 +317,28 @@ struct std_assoc ele_cap_to_computer_sys .assoc_class = "CIM_ElementCapabilities", .handler = cap_to_cs, + .make_ref = make_ref +}; + +struct std_assoc alloc_cap_to_resource_pool = { + .source_class = "CIM_AllocationCapabilities", + .source_prop = "Capabilities", + + .target_class = "CIM_ResourcePool", + .target_prop = "ManagedElement", + + .handler = alloc_to_pool, + .make_ref = make_ref +}; + +struct std_assoc resource_pool_to_alloc_cap = { + .source_class = "CIM_ResourcePool", + .source_prop = "ManagedElement", + + .target_class = "CIM_AllocationCapabilities", + .target_prop = "Capabilities", + + .handler = pool_to_alloc, .make_ref = make_ref }; @@ -282,6 +348,8 @@ struct std_assoc *assoc_handlers[] = { &system_to_vsm_cap, &vsm_cap_to_system, &ele_cap_to_computer_system, + &alloc_cap_to_resource_pool, + &resource_pool_to_alloc_cap, NULL };
Jay Gagnon wrote:
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1194557532 18000 # Node ID cf0a2938ba0b9d2e7af8a5790d0e56cc7564df73 # Parent 2d09468e2de7ed9194907bfff1efbb16ba40202d Add another relationship to ElementCapabilities, the ResourcePool-AllocationCapabilities connection. Only supports the pool_to_alloc direction, because it is the only one that's terribly interesting, but the other direction will need to be added at some point. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com>
+ + inst = get_typed_instance(_BROKER, "AllocationCapabilities", + NAMESPACE(ref)); + CMSetProperty(inst, "InstanceID", inst_id, CMPI_chars); + + ret = cu_get_u16_path(ref, "ResourceType", &type); + if (ret != 1) { + CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Could not get ResourceType."); + goto out; + } + CMSetProperty(inst, "ResourceType", &type, CMPI_uint16); + + inst_list_add(list, inst); + + out: + free(inst_id); + + return s; +}
Just a nit-picky thing here... why not call the AllocCapabilities function that setups the instance? I think using the same InstanceID for the AllocCapa as the pool might not always return the correct instance because AllocCapa uses <pool type>/0 as the InstanceID. But something like the NetworkingPool returns "NetworkPool/xenbr0". So you'll be creating an instance with "NetworkPool/xenbr0" as the InstanceID, which would conflict with what EnumInstances from AllocCapa would return. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
KR> Just a nit-picky thing here... why not call the AllocCapabilities KR> function that setups the instance? I think using the same KR> InstanceID for the AllocCapa as the pool might not always return KR> the correct instance because AllocCapa uses <pool type>/0 as the KR> InstanceID. But something like the NetworkingPool returns KR> "NetworkPool/xenbr0". So you'll be creating an instance with KR> "NetworkPool/xenbr0" as the InstanceID, which would conflict with KR> what EnumInstances from AllocCapa would return. I think AllocationCapabilities needs to change here. It think using the InstanceID from the pool makes the most sense. I'm not sure what the existing AllocationCapabilities stuff intended, but I'm assuming it will need to change to match this new stuff. Perhaps it was a hold-over from the days of one-pool-per-resource-type? Good catch, by the way :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
KR> Just a nit-picky thing here... why not call the AllocCapabilities KR> function that setups the instance? I think using the same KR> InstanceID for the AllocCapa as the pool might not always return KR> the correct instance because AllocCapa uses <pool type>/0 as the KR> InstanceID. But something like the NetworkingPool returns KR> "NetworkPool/xenbr0". So you'll be creating an instance with KR> "NetworkPool/xenbr0" as the InstanceID, which would conflict with KR> what EnumInstances from AllocCapa would return.
I think AllocationCapabilities needs to change here. It think using the InstanceID from the pool makes the most sense. I'm not sure what the existing AllocationCapabilities stuff intended, but I'm assuming it will need to change to match this new stuff. Perhaps it was a hold-over from the days of one-pool-per-resource-type?
Good catch, by the way :)
Just to be clear, copying InstanceID from the instance of ResourcePool to the instance of AllocationCapabilities is okay, but I need to change the AllocationCapabilities provider? That doesn't much surprise me; the AllocationCapabilities provider itself only exists so I could register it as a class; I'm honestly not even sure what it's supposed to do on its own . Reading through the entire Allocation Capabilities Profile (DSP1043) provides no insight into what get/enumerateInstance(s) would do. Reasonable detail is given about how it should be connected to things via associations, but there isn't much concept of a standalone instance. -- -Jay
JG> I'm honestly not even sure what it's supposed to do on its own . JG> Reading through the entire Allocation Capabilities Profile JG> (DSP1043) provides no insight into what get/enumerateInstance(s) JG> would do. Reasonable detail is given about how it should be JG> connected to things via associations, but there isn't much concept JG> of a standalone instance. The instance functions for AllocationCapabilities could probably be left unimplemented without anyone noticing, I imagine. However, I think that EnumInstances should return an instance of every AllocationCapabilities that could possibly be had through association with a pool. Thus, enumerate all the pools, create an AC instance for each and return them. GetInstance works the same. Should be trivial. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> I'm honestly not even sure what it's supposed to do on its own . JG> Reading through the entire Allocation Capabilities Profile JG> (DSP1043) provides no insight into what get/enumerateInstance(s) JG> would do. Reasonable detail is given about how it should be JG> connected to things via associations, but there isn't much concept JG> of a standalone instance.
The instance functions for AllocationCapabilities could probably be left unimplemented without anyone noticing, I imagine. However, I think that EnumInstances should return an instance of every AllocationCapabilities that could possibly be had through association with a pool. Thus, enumerate all the pools, create an AC instance for each and return them. GetInstance works the same. Should be trivial.
I can see what you mean with EnumInstances, although if this is how the DMTF wants things done I still take issue with it since it just feels like a broken way of doing what the associations do. -- -Jay
JG> +static CMPIStatus alloc_to_pool(const CMPIObjectPath *ref, JG> + struct std_assoc_info *info, JG> + struct inst_list *list) JG> +{ JG> + /* Pool to alloc is more important. That will be done first. */ JG> + return (CMPIStatus){CMPI_RC_OK, NULL}; JG> +} If we're going to leave this out for now, this should instead be RETURN_UNSUPPORTED(). However, since the InstanceID of a pool and an AllocationCapabilities are the same, isn't this behavior the same as below? If you set the below function to be the handler for the alloc_to_pool case, won't it behave as expected? JG> +static CMPIStatus pool_to_alloc(const CMPIObjectPath *ref, JG> + struct std_assoc_info *info, JG> + struct inst_list *list) JG> +{ JG> + int ret; JG> + char *inst_id; JG> + uint16_t type; JG> + CMPIInstance *inst = NULL; JG> + CMPIStatus s = {CMPI_RC_OK}; JG> + JG> + inst_id = cu_get_str_path(ref, "InstanceID"); JG> + if (inst_id == NULL) { JG> + CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, JG> + "Could not get InstanceID."); JG> + goto out; JG> + } JG> + JG> + inst = get_typed_instance(_BROKER, "AllocationCapabilities", JG> + NAMESPACE(ref)); JG> + CMSetProperty(inst, "InstanceID", inst_id, CMPI_chars); JG> + JG> + ret = cu_get_u16_path(ref, "ResourceType", &type); JG> + if (ret != 1) { JG> + CMSetStatusWithChars(_BROKER, &s, CMPI_RC_ERR_FAILED, JG> + "Could not get ResourceType."); JG> + goto out; JG> + } JG> + CMSetProperty(inst, "ResourceType", &type, CMPI_uint16); JG> + JG> + inst_list_add(list, inst); JG> + JG> + out: JG> + free(inst_id); JG> + JG> + return s; JG> +} This brings up an idea. Maybe it would be helpful for libcmpiutil to have a function that copies a value from a ref to an instance, or from an instance to an instance. This would assume that they're the same type, of course, but the above would be much shorter if we had something like: cu_copy_key(&ref, &inst, "InstanceID"); cu_copy_key(&ref, &inst, "ResourceType"); Right? I know there are a couple other places where we need to do that, so it might be worth knocking something up. Should be trivial and easy. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (4)
-
Dan Smith -
Daniel Veillard -
Jay Gagnon -
Kaitlin Rupert