[PATCH 0 of 2] #2 - SDC: Fixed type of returned instance and error code
Patch 1: The returned RASD instances have been of the general classname ResourceAllocationSettingData instead of the type specific one, e.g. ProcResourceAllocationSettingData Patch 2: RASD to AllocationCapabilities should return NOT_SUPPORTED instead of OK Diff to #1: patch 1: fixed missing check for unknown resource type
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1197543884 -3600 # Node ID 733f1e73fe50ea102d9c8942e3dfb0bbca2e6f91 # Parent 86999a1e8fac6f6cee5ca62a4005ca37759713ff SDC: returned instances are not of type specific subclass Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 86999a1e8fac -r 733f1e73fe50 src/Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c Wed Dec 12 17:22:06 2007 -0800 +++ b/src/Virt_SettingsDefineCapabilities.c Thu Dec 13 12:04:44 2007 +0100 @@ -658,6 +658,7 @@ static CMPIInstance *sdc_rasd_inst(const struct sdc_rasd_prop *prop_list; int i; char *inst_id; + char *base = NULL; uint16_t resource_type; /* Defaults for the following are from CIM_SettingsDefineCapabilities.mof. */ @@ -704,9 +705,29 @@ static CMPIInstance *sdc_rasd_inst(const if (s->rc != CMPI_RC_OK) goto out; + switch(rasd->resource_type) { + case CIM_RASD_TYPE_MEM: + base = "MemResourceAllocationSettingData"; + break; + case CIM_RASD_TYPE_PROC: + base = "ProcResourceAllocationSettingData"; + break; + case CIM_RASD_TYPE_NET: + base = "NetResourceAllocationSettingData"; + break; + case CIM_RASD_TYPE_DISK: + base = "DiskResourceAllocationSettingData"; + break; + default: + cu_statusf(broker, s, + CMPI_RC_ERR_FAILED, + "Resource type not known"); + goto err; + } + inst = get_typed_instance(broker, CLASSNAME(ref), - "ResourceAllocationSettingData", + base, NAMESPACE(ref)); CMSetProperty(inst, "InstanceID", inst_id, CMPI_chars); @@ -723,6 +744,7 @@ static CMPIInstance *sdc_rasd_inst(const prop_list[i].value, prop_list[i].type); } + err: CU_DEBUG("freeing prop_list"); free_rasd_prop_list(prop_list); out:
HE> + char *base = NULL; This should be a const char *. HE> + switch(rasd->resource_type) { HE> + case CIM_RASD_TYPE_MEM: HE> + base = "MemResourceAllocationSettingData"; HE> + break; HE> + case CIM_RASD_TYPE_PROC: HE> + base = "ProcResourceAllocationSettingData"; HE> + break; HE> + case CIM_RASD_TYPE_NET: HE> + base = "NetResourceAllocationSettingData"; HE> + break; HE> + case CIM_RASD_TYPE_DISK: HE> + base = "DiskResourceAllocationSettingData"; HE> + break; HE> + default: HE> + cu_statusf(broker, s, HE> + CMPI_RC_ERR_FAILED, HE> + "Resource type not known"); HE> + goto err; HE> + } HE> + Maybe this should be a helper function exposed in Virt_RASD.h, instead of buried in SDC? -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> + char *base = NULL;
This should be a const char *.
Good catch.
HE> + switch(rasd->resource_type) { HE> + case CIM_RASD_TYPE_MEM: HE> + base = "MemResourceAllocationSettingData"; HE> + break; HE> + case CIM_RASD_TYPE_PROC: HE> + base = "ProcResourceAllocationSettingData"; HE> + break; HE> + case CIM_RASD_TYPE_NET: HE> + base = "NetResourceAllocationSettingData"; HE> + break; HE> + case CIM_RASD_TYPE_DISK: HE> + base = "DiskResourceAllocationSettingData"; HE> + break; HE> + default: HE> + cu_statusf(broker, s, HE> + CMPI_RC_ERR_FAILED, HE> + "Resource type not known"); HE> + goto err; HE> + } HE> +
Maybe this should be a helper function exposed in Virt_RASD.h, instead of buried in SDC? A very good idea. I also did not like the length this function got due to this change and putting it into a helper of RASD makes much more sense. I will update the patch and resend it. Thanks.
-- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1197543894 -3600 # Node ID 346a3df2e978f42fc44e89c2d60e685befdda5ff # Parent 733f1e73fe50ea102d9c8942e3dfb0bbca2e6f91 SDC: RASD to AllocationCapabilities should return NOT_SUPPORTED instead of OK Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 733f1e73fe50 -r 346a3df2e978 src/Virt_SettingsDefineCapabilities.c --- a/src/Virt_SettingsDefineCapabilities.c Thu Dec 13 12:04:44 2007 +0100 +++ b/src/Virt_SettingsDefineCapabilities.c Thu Dec 13 12:04:54 2007 +0100 @@ -836,8 +836,8 @@ static CMPIStatus rasd_to_alloc_cap(cons struct std_assoc_info *info, struct inst_list *list) { - CMPIStatus s = {CMPI_RC_OK}; - + CMPIStatus s = {CMPI_RC_ERR_NOT_SUPPORTED}; + /* This direction of the association currently not supported. */ return s;
HE> - CMPIStatus s = {CMPI_RC_OK}; HE> - HE> + CMPIStatus s = {CMPI_RC_ERR_NOT_SUPPORTED}; HE> + HE> /* This direction of the association currently not supported. */ Not that it's a big deal or anything, but we do have RETURN_UNSUPPORTED() in libcmpiutil for this scenario. If you like it and want to re-spin the patch with it, that would be cool. Your call... :) Sorry I didn't get to reviewing this set until the second round... -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> - CMPIStatus s = {CMPI_RC_OK}; HE> - HE> + CMPIStatus s = {CMPI_RC_ERR_NOT_SUPPORTED}; HE> + HE> /* This direction of the association currently not supported. */
Not that it's a big deal or anything, but we do have RETURN_UNSUPPORTED() in libcmpiutil for this scenario. Sure, thanks for reminding me :). That's much better than setting the status by hand. Good catch. I will update the patch and resend with the changes of patch #1. If you like it and want to re-spin the patch with it, that would be cool. Your call... :)
Sorry I didn't get to reviewing this set until the second round... Nop.
-- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Heidi Eckhart wrote:
Patch 1: The returned RASD instances have been of the general classname ResourceAllocationSettingData instead of the type specific one, e.g. ProcResourceAllocationSettingData Patch 2: RASD to AllocationCapabilities should return NOT_SUPPORTED instead of OK
Diff to #1: patch 1: fixed missing check for unknown resource type
Excellent! I like this change. =) And the set tested just fine. +1 -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert