[PATCH 0 of 2] [RFC] Add property filtering to RASD.
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1203702578 28800 # Node ID 2bff66d74872c62ef38fca81fafd32f0d7fb126a # Parent d02f5729977381ec6f5aec3503dc5f970a428b88 Add property filtering to RASD provider. wbemcli is unable to parse embedded instance arrays, so this additional property filtering support will allow queries to exclude the HostResource property. Failing Query: wbemcli ei http://localhost/root/virt:Xen_ProcResourceAllocationSettingData * * wbemcli: Parse Exception: Unknown attribute in list for PROPERTY.ARRAY (EMBEDDEDOB) Passing Query: wbemcli ei http://localhost/root/virt:Xen_ProcResourceAllocationSettingData ResourceType Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r d02f57299773 -r 2bff66d74872 src/Virt_RASD.c --- a/src/Virt_RASD.c Fri Feb 22 12:54:52 2008 +0100 +++ b/src/Virt_RASD.c Fri Feb 22 09:49:38 2008 -0800 @@ -283,8 +283,10 @@ static CMPIInstance *rasd_from_vdev(cons static CMPIInstance *rasd_from_vdev(const CMPIBroker *broker, struct virt_device *dev, const char *host, - const CMPIObjectPath *ref) -{ + const CMPIObjectPath *ref, + const char **properties) +{ + CMPIStatus s; CMPIInstance *inst; uint16_t type; char *base; @@ -312,6 +314,14 @@ static CMPIInstance *rasd_from_vdev(cons NAMESPACE(ref)); if (inst == NULL) return inst; + + if (properties != NULL) { + const char *keys[] = {"InstanceID", NULL}; + s = CMSetPropertyFilter(inst, properties, keys); + /* FIXME - in case of an error, should we continue on? */ + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Unable to set property filter: %d", s.rc); + } id = get_fq_devid((char *)host, dev->id); @@ -361,6 +371,7 @@ CMPIStatus get_rasd_by_name(const CMPIBr const CMPIObjectPath *reference, const char *name, const uint16_t type, + const char **properties, CMPIInstance **_inst) { CMPIInstance *inst = NULL; @@ -397,7 +408,7 @@ CMPIStatus get_rasd_by_name(const CMPIBr goto out; } - inst = rasd_from_vdev(broker, dev, host, reference); + inst = rasd_from_vdev(broker, dev, host, reference, properties); if (inst == NULL) cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, @@ -415,6 +426,7 @@ CMPIStatus get_rasd_by_name(const CMPIBr CMPIStatus get_rasd_by_ref(const CMPIBroker *broker, const CMPIObjectPath *reference, + const char **properties, CMPIInstance **_inst) { CMPIStatus s = {CMPI_RC_OK, NULL}; @@ -435,7 +447,7 @@ CMPIStatus get_rasd_by_ref(const CMPIBro goto out; } - s = get_rasd_by_name(broker, reference, name, type, _inst); + s = get_rasd_by_name(broker, reference, name, type, properties, _inst); if (s.rc != CMPI_RC_OK) goto out; @@ -498,6 +510,7 @@ CMPIrc rasd_classname_from_type(uint16_t } static CMPIStatus _enum_rasds(const CMPIObjectPath *ref, + const char **properties, struct inst_list *list) { virConnectPtr conn = NULL; @@ -537,6 +550,7 @@ static CMPIStatus _enum_rasds(const CMPI virDomainGetName(domains[i]), types[j], ref, + properties, list); } virDomainFree(domains[i]); @@ -553,6 +567,7 @@ static CMPIStatus _enum_rasds(const CMPI static CMPIStatus return_enum_rasds(const CMPIObjectPath *ref, const CMPIResult *results, + const char **properties, const bool names_only) { struct inst_list list; @@ -560,7 +575,7 @@ static CMPIStatus return_enum_rasds(cons inst_list_init(&list); - s = _enum_rasds(ref, &list); + s = _enum_rasds(ref, properties, &list); if (s.rc == CMPI_RC_OK) { if (names_only) cu_return_instance_names(results, &list); @@ -578,7 +593,7 @@ static CMPIStatus EnumInstanceNames(CMPI const CMPIResult *results, const CMPIObjectPath *reference) { - return return_enum_rasds(reference, results, true); + return return_enum_rasds(reference, results, NULL, true); } static CMPIStatus EnumInstances(CMPIInstanceMI *self, @@ -588,7 +603,7 @@ static CMPIStatus EnumInstances(CMPIInst const char **properties) { - return return_enum_rasds(reference, results, false); + return return_enum_rasds(reference, results, properties, false); } static CMPIStatus GetInstance(CMPIInstanceMI *self, @@ -600,7 +615,7 @@ static CMPIStatus GetInstance(CMPIInstan CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *inst = NULL; - s = get_rasd_by_ref(_BROKER, ref, &inst); + s = get_rasd_by_ref(_BROKER, ref, properties, &inst); if (s.rc != CMPI_RC_OK) goto out; @@ -614,6 +629,7 @@ int rasds_for_domain(const CMPIBroker *b const char *name, const uint16_t type, const CMPIObjectPath *ref, + const char **properties, struct inst_list *_list) { struct virt_device *list; @@ -631,7 +647,7 @@ int rasds_for_domain(const CMPIBroker *b for (i = 0; i < count; i++) { CMPIInstance *inst; - inst = rasd_from_vdev(broker, &list[i], name, ref); + inst = rasd_from_vdev(broker, &list[i], name, ref, properties); if (inst != NULL) inst_list_add(_list, inst); } diff -r d02f57299773 -r 2bff66d74872 src/Virt_RASD.h --- a/src/Virt_RASD.h Fri Feb 22 12:54:52 2008 +0100 +++ b/src/Virt_RASD.h Fri Feb 22 09:49:38 2008 -0800 @@ -37,6 +37,7 @@ int rasds_for_domain(const CMPIBroker *b const char *name, const uint16_t type, const CMPIObjectPath *ref, + const char **properties, struct inst_list *_list); CMPIrc rasd_type_from_classname(const char *cn, uint16_t *type); @@ -46,10 +47,12 @@ CMPIStatus get_rasd_by_name(const CMPIBr const CMPIObjectPath *reference, const char *name, const uint16_t type, + const char **properties, CMPIInstance **_inst); CMPIStatus get_rasd_by_ref(const CMPIBroker *broker, const CMPIObjectPath *reference, + const char **properties, CMPIInstance **_inst); #endif
KR> + const char *keys[] = {"InstanceID", NULL}; KR> + s = CMSetPropertyFilter(inst, properties, keys); KR> + /* FIXME - in case of an error, should we continue on? */ I think we should do exactly what you have: log a message and continue on. I doubt much more will work if this part fails (due to memory or something), so the error will be caught later. If it's unrelated and the rest can proceed, I think that's better than dying here, since the property filter is really just an optimization. Since we don't do it anywhere else, a failure here will just make RASD behave like everything else :) +1 from me. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Kaitlin Rupert wrote:
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1203702578 28800 # Node ID 2bff66d74872c62ef38fca81fafd32f0d7fb126a # Parent d02f5729977381ec6f5aec3503dc5f970a428b88 Add property filtering to RASD provider.
wbemcli is unable to parse embedded instance arrays, so this additional property filtering support will allow queries to exclude the HostResource property.
Failing Query: wbemcli ei http://localhost/root/virt:Xen_ProcResourceAllocationSettingData * * wbemcli: Parse Exception: Unknown attribute in list for PROPERTY.ARRAY (EMBEDDEDOB)
Passing Query: wbemcli ei http://localhost/root/virt:Xen_ProcResourceAllocationSettingData ResourceType
Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com>
diff -r d02f57299773 -r 2bff66d74872 src/Virt_RASD.c --- a/src/Virt_RASD.c Fri Feb 22 12:54:52 2008 +0100 +++ b/src/Virt_RASD.c Fri Feb 22 09:49:38 2008 -0800 @@ -312,6 +314,14 @@ static CMPIInstance *rasd_from_vdev(cons NAMESPACE(ref)); if (inst == NULL) return inst; + + if (properties != NULL) { + const char *keys[] = {"InstanceID", NULL}; + s = CMSetPropertyFilter(inst, properties, keys); + /* FIXME - in case of an error, should we continue on? */ + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Unable to set property filter: %d", s.rc); + }
That's a very good point to enable the provider interfaces for the property filtering :). I think it makes sense to move this code into get_typed_instance() of libxkutil, to avoid implementing these lines for every provider. What's then only necessary is routing the properties list through the interfaces. But you will now think ... and how about the keys ? Well, that's an interesting thing. The CMPI 2.0 spec tells the following about the keys: CMPI 2.0 Spec; "The keyList argument shall be ignored by the MB; it is here to maintain binary compatibility with previous specifications. Providers should explicitly set the key names and values via the CMPIInstanceMI.setObjectPath() function." Translated I think this means, that we can ignore the keys by setting it to NULL. Do you agree ? You can also skip the check if a the properties filter is there, as NULL is interpreted as "all properties accepted". CMPI 2.0 Spec: "The propertyList argument defines the properties that will be accepted by subsequent setProperty() operations. The propertyList argument is an array of pointers to character strings, terminated by a NULL pointer. A NULL value effectively means that all properties will be accepted. A pointer to an empty list means that no properties will be accepted."
CMPIStatus get_rasd_by_ref(const CMPIBroker *broker, const CMPIObjectPath *reference, + const char **properties, CMPIInstance **_inst) { CMPIStatus s = {CMPI_RC_OK, NULL}; @@ -435,7 +447,7 @@ CMPIStatus get_rasd_by_ref(const CMPIBro goto out; }
- s = get_rasd_by_name(broker, reference, name, type, _inst); + s = get_rasd_by_name(broker, reference, name, type, properties, _inst);
Sorry, I broker your patch with my previous changes at this line. -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
HE> I think it makes sense to move this code into get_typed_instance() HE> of libxkutil, to avoid implementing these lines for every HE> provider. What's then only necessary is routing the properties HE> list through the interfaces. Yes, I think that's a good idea, but it will require a lot of silly changes, so it's probably best to wait until after we finish with some of our more important correctness issues. Thanks! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> I think it makes sense to move this code into get_typed_instance() HE> of libxkutil, to avoid implementing these lines for every HE> provider. What's then only necessary is routing the properties HE> list through the interfaces.
Yes, I think that's a good idea, but it will require a lot of silly changes, so it's probably best to wait until after we finish with some of our more important correctness issues.
Absolutely ! I can only agree. -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
+ + if (properties != NULL) { + const char *keys[] = {"InstanceID", NULL}; + s = CMSetPropertyFilter(inst, properties, keys); + /* FIXME - in case of an error, should we continue on? */ + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Unable to set property filter: %d", s.rc); + }
That's a very good point to enable the provider interfaces for the property filtering :). I think it makes sense to move this code into get_typed_instance() of libxkutil, to avoid implementing these lines for every provider. What's then only necessary is routing the properties list through the interfaces. But you will now think ... and how about the keys ? Well, that's an interesting thing. The CMPI 2.0 spec tells the following about the keys: CMPI 2.0 Spec; "The keyList argument shall be ignored by the MB; it is here to maintain binary compatibility with previous specifications. Providers should explicitly set the key names and values via the CMPIInstanceMI.setObjectPath() function." Translated I think this means, that we can ignore the keys by setting it to NULL. Do you agree ?
I tried using NULL for the keys based on the same line from the CMPI spec, but this crashed the provider. I might be missing something though.
You can also skip the check if a the properties filter is there, as NULL is interpreted as "all properties accepted". CMPI 2.0 Spec: "The propertyList argument defines the properties that will be accepted by subsequent setProperty() operations. The propertyList argument is an array of pointers to character strings, terminated by a NULL pointer. A NULL value effectively means that all properties will be accepted. A pointer to an empty list means that no properties will be accepted."
Ah, good to know. I'll remove this check and rebase the patch on your recent changes. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
Kaitlin Rupert wrote:
+ + if (properties != NULL) { + const char *keys[] = {"InstanceID", NULL}; + s = CMSetPropertyFilter(inst, properties, keys); + /* FIXME - in case of an error, should we continue on? */ + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Unable to set property filter: %d", s.rc); + }
That's a very good point to enable the provider interfaces for the property filtering :). I think it makes sense to move this code into get_typed_instance() of libxkutil, to avoid implementing these lines for every provider. What's then only necessary is routing the properties list through the interfaces. But you will now think ... and how about the keys ? Well, that's an interesting thing. The CMPI 2.0 spec tells the following about the keys: CMPI 2.0 Spec; "The keyList argument shall be ignored by the MB; it is here to maintain binary compatibility with previous specifications. Providers should explicitly set the key names and values via the CMPIInstanceMI.setObjectPath() function." Translated I think this means, that we can ignore the keys by setting it to NULL. Do you agree ?
I tried using NULL for the keys based on the same line from the CMPI spec, but this crashed the provider. I might be missing something though. I've tested it with Pegasus and sfcb: Pegasus returns with FAILED and does not set the propertylist sfcb is crashing So my interpretation of this description seem to be wrong and the keyList is needed. Thanks for evaluating this.
-- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
Heidi Eckhart wrote:
Kaitlin Rupert wrote:
+ + if (properties != NULL) { + const char *keys[] = {"InstanceID", NULL}; + s = CMSetPropertyFilter(inst, properties, keys); + /* FIXME - in case of an error, should we continue on? */ + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Unable to set property filter: %d", s.rc); + }
That's a very good point to enable the provider interfaces for the property filtering :). I think it makes sense to move this code into get_typed_instance() of libxkutil, to avoid implementing these lines for every provider. What's then only necessary is routing the properties list through the interfaces. But you will now think ... and how about the keys ? Well, that's an interesting thing. The CMPI 2.0 spec tells the following about the keys: CMPI 2.0 Spec; "The keyList argument shall be ignored by the MB; it is here to maintain binary compatibility with previous specifications. Providers should explicitly set the key names and values via the CMPIInstanceMI.setObjectPath() function." Translated I think this means, that we can ignore the keys by setting it to NULL. Do you agree ?
I tried using NULL for the keys based on the same line from the CMPI spec, but this crashed the provider. I might be missing something though. I've tested it with Pegasus and sfcb: Pegasus returns with FAILED and does not set the propertylist sfcb is crashing So my interpretation of this description seem to be wrong and the keyList is needed. Thanks for evaluating this.
Thanks for confirming this Heidi. I was confused by the behavior, and thought that maybe I was calling the function incorrectly. =) -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1203702667 28800 # Node ID 743b306e9bc680cab97c2afb60dfe9547e1d182c # Parent 2bff66d74872c62ef38fca81fafd32f0d7fb126a Update providers that call rasd_for_domain() and get_rasd_instance(). Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 2bff66d74872 -r 743b306e9bc6 src/Virt_ElementSettingData.c --- a/src/Virt_ElementSettingData.c Fri Feb 22 09:49:38 2008 -0800 +++ b/src/Virt_ElementSettingData.c Fri Feb 22 09:51:07 2008 -0800 @@ -98,7 +98,7 @@ static CMPIStatus rasd_to_rasd(const CMP /* Special association case: * RASD instance is pointing to itself */ - s = get_rasd_by_ref(_BROKER, ref, &inst); + s = get_rasd_by_ref(_BROKER, ref, info->properties, &inst); if (s.rc != CMPI_RC_OK) goto out; diff -r 2bff66d74872 -r 743b306e9bc6 src/Virt_ResourceAllocationFromPool.c --- a/src/Virt_ResourceAllocationFromPool.c Fri Feb 22 09:49:38 2008 -0800 +++ b/src/Virt_ResourceAllocationFromPool.c Fri Feb 22 09:51:07 2008 -0800 @@ -66,7 +66,7 @@ static CMPIStatus rasd_to_pool(const CMP goto out; } - s = get_rasd_by_name(_BROKER, ref, id, type, &inst); + s = get_rasd_by_name(_BROKER, ref, id, type, NULL, &inst); if (s.rc != CMPI_RC_OK) goto out; @@ -138,6 +138,7 @@ static int rasds_from_pool(uint16_t type static int rasds_from_pool(uint16_t type, const CMPIObjectPath *ref, const char *poolid, + const char **properties, struct inst_list *list) { CMPIStatus s; @@ -164,6 +165,7 @@ static int rasds_from_pool(uint16_t type name, type, ref, + properties, &tmp); filter_by_pool(list, &tmp, poolid); @@ -213,6 +215,7 @@ static CMPIStatus pool_to_rasd(const CMP rasds_from_pool(type, ref, poolid, + info->properties, list); cu_statusf(_BROKER, &s, diff -r 2bff66d74872 -r 743b306e9bc6 src/Virt_SettingsDefineState.c --- a/src/Virt_SettingsDefineState.c Fri Feb 22 09:49:38 2008 -0800 +++ b/src/Virt_SettingsDefineState.c Fri Feb 22 09:51:07 2008 -0800 @@ -98,6 +98,7 @@ static CMPIStatus dev_to_rasd(const CMPI name, device_type_from_classname(CLASSNAME(ref)), ref, + info->properties, &rasds); rasd = find_rasd(&rasds, id); @@ -180,7 +181,7 @@ static CMPIStatus rasd_to_dev(const CMPI goto out; } - s = get_rasd_by_name(_BROKER, ref, id, type, &inst); + s = get_rasd_by_name(_BROKER, ref, id, type, NULL, &inst); if (s.rc != CMPI_RC_OK) goto out; diff -r 2bff66d74872 -r 743b306e9bc6 src/Virt_VSSDComponent.c --- a/src/Virt_VSSDComponent.c Fri Feb 22 09:49:38 2008 -0800 +++ b/src/Virt_VSSDComponent.c Fri Feb 22 09:51:07 2008 -0800 @@ -66,6 +66,7 @@ static CMPIStatus vssd_to_rasd(const CMP name, types[i], ref, + info->properties, list); } diff -r 2bff66d74872 -r 743b306e9bc6 src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c Fri Feb 22 09:49:38 2008 -0800 +++ b/src/Virt_VirtualSystemManagementService.c Fri Feb 22 09:51:07 2008 -0800 @@ -1041,7 +1041,7 @@ static CMPIStatus rasd_refs_to_insts(con continue; } - s = get_rasd_by_name(_BROKER, reference, id, type, &inst); + s = get_rasd_by_name(_BROKER, reference, id, type, NULL, &inst); if (s.rc != CMPI_RC_OK) continue;
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert