[PATCH 0 of 5] #2 EAFP and RAFP support invalid references.
Updates from last set: -"Cleanup device_type_from_poolid() in DevicePool" - reverted the function name back to device_type_from_poolid() instead of device_type_from_str() -Added two new patches: -"GetInstance() in DevicePool returns an instance for invalid refs." -"Roll DevicePool GetInstance() functionality into a seperate function." -Re-worked EAFP and RAFP patches based on the two new patches. This fix makes it so that EAFP and RAFP do not return instances if an invalid ref is given. These patches are dependant on the "Clean up RAFP and EAFP pool_to_* handlers." patchset.
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198259651 28800 # Node ID 97c1cab36073319f7cf78cbec70eec9eaee01a4d # Parent 84eb3549e19956d912684a09d58631211724a720 Cleanup device_type_from_poolid() in DevicePool. Change return type to uint16_t to match the device type. Change "Memory" and "Processor" to "MemoryPool" and "ProcessorPool" so that it's more strict. Replace strstr with STARTS_WITH() so that the check is more strict. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 84eb3549e199 -r 97c1cab36073 src/Virt_DevicePool.c --- a/src/Virt_DevicePool.c Thu Dec 20 10:14:00 2007 -0800 +++ b/src/Virt_DevicePool.c Fri Dec 21 09:54:11 2007 -0800 @@ -268,15 +268,15 @@ char *pool_member_of(const CMPIBroker *b return poolid; } -int device_type_from_poolid(const char *id) -{ - if (strstr(id, "NetworkPool")) +uint16_t device_type_from_poolid(const char *id) +{ + if (STARTS_WITH(id, "NetworkPool")) return VIRT_DEV_NET; - else if (strstr(id, "DiskPool")) + else if (STARTS_WITH(id, "DiskPool")) return VIRT_DEV_DISK; - else if (strstr(id, "Memory")) + else if (STARTS_WITH(id, "MemoryPool")) return VIRT_DEV_MEM; - else if (strstr(id, "Processor")) + else if (STARTS_WITH(id, "ProcessorPool")) return VIRT_DEV_VCPU; else return VIRT_DEV_UNKNOWN; diff -r 84eb3549e199 -r 97c1cab36073 src/Virt_DevicePool.h --- a/src/Virt_DevicePool.h Thu Dec 20 10:14:00 2007 -0800 +++ b/src/Virt_DevicePool.h Fri Dec 21 09:54:11 2007 -0800 @@ -56,11 +56,11 @@ char *pool_member_of(const CMPIBroker *b const char *id); /** - * * Get the device type of a given pool from the pool's InstanceID - * * - * * @param id The InstanceID of the pool - * */ -int device_type_from_poolid(const char *id); + * Get the device type of a given pool from the pool's InstanceID + * + * @param id The InstanceID of the pool + */ +uint16_t device_type_from_poolid(const char *id); /** * Get all device pools on the system for the given connection
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198261308 28800 # Node ID 40de750e13d37994bee0f7031e82f612d02ebe4d # Parent 97c1cab36073319f7cf78cbec70eec9eaee01a4d GetInstance() in DevicePool returns an instance for invalid refs. If a ref with a correct InstanceID but non-matching classname is passed, GetInstance() returns an instance that matches the InstanceID. This should return an error. Add cu_compare_ref() after retrieving the instance to make sure ref is valid. Failing query: wbemcli gi 'http://localhost:5988/root/virt:Xen_ProcessorPool.InstanceID="MemoryPool/0"' Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 97c1cab36073 -r 40de750e13d3 src/Virt_DevicePool.c --- a/src/Virt_DevicePool.c Fri Dec 21 09:54:11 2007 -0800 +++ b/src/Virt_DevicePool.c Fri Dec 21 10:21:48 2007 -0800 @@ -739,6 +739,7 @@ static CMPIStatus GetInstance(CMPIInstan CMPIInstance *inst; virConnectPtr conn = NULL; const char *id = NULL; + const char *prop; if (cu_get_str_path(reference, "InstanceID", &id) != CMPI_RC_OK) { cu_statusf(_BROKER, &s, @@ -753,6 +754,12 @@ static CMPIStatus GetInstance(CMPIInstan inst = get_pool_by_id(_BROKER, conn, id, NAMESPACE(reference)); if (inst) { + prop = cu_compare_ref(reference, inst); + if (prop != NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such ResourcePool instance (%s)", prop); + } CMReturnInstance(results, inst); CMSetStatus(&s, CMPI_RC_OK); } else {
Kaitlin Rupert wrote:
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198261308 28800 # Node ID 40de750e13d37994bee0f7031e82f612d02ebe4d # Parent 97c1cab36073319f7cf78cbec70eec9eaee01a4d GetInstance() in DevicePool returns an instance for invalid refs.
If a ref with a correct InstanceID but non-matching classname is passed, GetInstance() returns an instance that matches the InstanceID. This should return an error.
Add cu_compare_ref() after retrieving the instance to make sure ref is valid.
Failing query: wbemcli gi 'http://localhost:5988/root/virt:Xen_ProcessorPool.InstanceID="MemoryPool/0"'
Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com>
inst = get_pool_by_id(_BROKER, conn, id, NAMESPACE(reference)); if (inst) { + prop = cu_compare_ref(reference, inst); + if (prop != NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such ResourcePool instance (%s)", prop); + }
Just want to make sure I understand something correctly. You don't need to free prop here because cu_compare_ref gets it using CMGetCharPtr, right? That memory is managed by the CIMOM for you? -- -Jay
Jay Gagnon wrote:
Kaitlin Rupert wrote:
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198261308 28800 # Node ID 40de750e13d37994bee0f7031e82f612d02ebe4d # Parent 97c1cab36073319f7cf78cbec70eec9eaee01a4d GetInstance() in DevicePool returns an instance for invalid refs.
If a ref with a correct InstanceID but non-matching classname is passed, GetInstance() returns an instance that matches the InstanceID. This should return an error.
Add cu_compare_ref() after retrieving the instance to make sure ref is valid.
Failing query: wbemcli gi 'http://localhost:5988/root/virt:Xen_ProcessorPool.InstanceID="MemoryPool/0"'
Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com>
inst = get_pool_by_id(_BROKER, conn, id, NAMESPACE(reference)); if (inst) { + prop = cu_compare_ref(reference, inst); + if (prop != NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such ResourcePool instance (%s)", prop); + }
Just want to make sure I understand something correctly. You don't need to free prop here because cu_compare_ref gets it using CMGetCharPtr, right? That memory is managed by the CIMOM for you?
Yes, that's correct. It's the same reason why you don't need to free an instance that's been created using CMNewInstance() (etc). -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198262431 28800 # Node ID 58166b55f2b923859186c9d4bab47778c209e032 # Parent 40de750e13d37994bee0f7031e82f612d02ebe4d Roll DevicePool GetInstance() functionality into a seperate function. Add this function to DevicePool.h so that other providers can use this to get a ResourcePool instance. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 40de750e13d3 -r 58166b55f2b9 src/Virt_DevicePool.c --- a/src/Virt_DevicePool.c Fri Dec 21 10:21:48 2007 -0800 +++ b/src/Virt_DevicePool.c Fri Dec 21 10:40:31 2007 -0800 @@ -712,47 +712,28 @@ static CMPIStatus return_pool(const CMPI return s; } -static CMPIStatus EnumInstanceNames(CMPIInstanceMI *self, - const CMPIContext *context, - const CMPIResult *results, - const CMPIObjectPath *reference) -{ - return return_pool(reference, results, true, false); -} - -static CMPIStatus EnumInstances(CMPIInstanceMI *self, - const CMPIContext *context, - const CMPIResult *results, - const CMPIObjectPath *reference, - const char **properties) -{ - return return_pool(reference, results, false, false); -} - -static CMPIStatus GetInstance(CMPIInstanceMI *self, - const CMPIContext *context, - const CMPIResult *results, - const CMPIObjectPath *reference, - const char **properties) +CMPIStatus get_pool_inst(const CMPIBroker *broker, + const CMPIObjectPath *reference, + CMPIInstance **instance) { CMPIStatus s; - CMPIInstance *inst; + CMPIInstance *inst = NULL; virConnectPtr conn = NULL; const char *id = NULL; const char *prop; if (cu_get_str_path(reference, "InstanceID", &id) != CMPI_RC_OK) { - cu_statusf(_BROKER, &s, + cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, "Missing InstanceID"); goto out; } - conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s); + conn = connect_by_classname(broker, CLASSNAME(reference), &s); if (conn == NULL) goto out; - inst = get_pool_by_id(_BROKER, conn, id, NAMESPACE(reference)); + inst = get_pool_by_id(broker, conn, id, NAMESPACE(reference)); if (inst) { prop = cu_compare_ref(reference, inst); if (prop != NULL) { @@ -760,17 +741,48 @@ static CMPIStatus GetInstance(CMPIInstan CMPI_RC_ERR_NOT_FOUND, "No such ResourcePool instance (%s)", prop); } - CMReturnInstance(results, inst); - CMSetStatus(&s, CMPI_RC_OK); } else { - cu_statusf(_BROKER, &s, + cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, "No such instance `%s'", id); } - out: virConnectClose(conn); + *instance = inst; + + return s; +} + +static CMPIStatus EnumInstanceNames(CMPIInstanceMI *self, + const CMPIContext *context, + const CMPIResult *results, + const CMPIObjectPath *reference) +{ + return return_pool(reference, results, true, false); +} + +static CMPIStatus EnumInstances(CMPIInstanceMI *self, + const CMPIContext *context, + const CMPIResult *results, + const CMPIObjectPath *reference, + const char **properties) +{ + return return_pool(reference, results, false, false); +} + +static CMPIStatus GetInstance(CMPIInstanceMI *self, + const CMPIContext *context, + const CMPIResult *results, + const CMPIObjectPath *reference, + const char **properties) +{ + CMPIStatus s; + CMPIInstance *inst = NULL; + + s = get_pool_inst(_BROKER, reference, &inst); + if ((s.rc == CMPI_RC_OK) && (inst != NULL)) + CMReturnInstance(results, inst); return s; } diff -r 40de750e13d3 -r 58166b55f2b9 src/Virt_DevicePool.h --- a/src/Virt_DevicePool.h Fri Dec 21 10:21:48 2007 -0800 +++ b/src/Virt_DevicePool.h Fri Dec 21 10:40:31 2007 -0800 @@ -74,6 +74,18 @@ CMPIStatus get_all_pools(const CMPIBroke virConnectPtr conn, const char *ns, struct inst_list *list); + +/** + * Get a device pools instance for the given reference + * + * @param broker The current Broker + * @param reference The reference passed to the CIMOM + * @param instance Return corresponding instance + */ +CMPIStatus get_pool_inst(const CMPIBroker *broker, + const CMPIObjectPath *reference, + CMPIInstance **instance); + #endif /*
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198263076 28800 # Node ID e029eb82cf076cdf046bdbbfdcc099fef3ec0ec2 # Parent 58166b55f2b923859186c9d4bab47778c209e032 Add check in EAFP to ensure ref passed is an actual instance. EAFP currently returns instances for invalid refs where the InstanceID doesn't match the classname type (see failing query). Get the ResourcePool instance that corresponds to the ref using get_pool_inst(). If this fails, the reference is not valid. Failing query: wbemcli ain -ac Xen_ElementAllocatedFromPool 'http://localhost:5988/root/virt:Xen_DiskPool.InstanceID="NetworkPool/virbr0"' Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 58166b55f2b9 -r e029eb82cf07 src/Virt_ElementAllocatedFromPool.c --- a/src/Virt_ElementAllocatedFromPool.c Fri Dec 21 10:40:31 2007 -0800 +++ b/src/Virt_ElementAllocatedFromPool.c Fri Dec 21 10:51:16 2007 -0800 @@ -202,6 +202,7 @@ static CMPIStatus pool_to_vdev(const CMP const char *poolid; CMPIStatus s = {CMPI_RC_OK, NULL}; uint16_t type; + CMPIInstance *inst; if (!match_hypervisor_prefix(ref, info)) return s; @@ -223,6 +224,9 @@ static CMPIStatus pool_to_vdev(const CMP goto out; } + s = get_pool_inst(_BROKER, ref, &inst); + if ((s.rc != CMPI_RC_OK) || (inst == NULL)) + goto out; devs_from_pool(type, ref, poolid, list);
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198263517 28800 # Node ID 3ea63b2076c1d02b7c3c48910809b9fb11ee290f # Parent e029eb82cf076cdf046bdbbfdcc099fef3ec0ec2 Add check in RAFP to ensure ref passed is an actual instance. RAFP currently returns instances for invalid refs where the InstanceID doesn't match the classname type (see failing query). Get the ResourcePool instance that corresponds to the ref using get_pool_inst(). If this fails, the reference is not valid. Failing query: wbemcli ain -ac Xen_ResourceAllocationFromPool 'http://localhost:5988/root/virt:Xen_ProcessorPool.InstanceID="MemoryPool/0"' Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r e029eb82cf07 -r 3ea63b2076c1 src/Virt_ResourceAllocationFromPool.c --- a/src/Virt_ResourceAllocationFromPool.c Fri Dec 21 10:51:16 2007 -0800 +++ b/src/Virt_ResourceAllocationFromPool.c Fri Dec 21 10:58:37 2007 -0800 @@ -183,6 +183,7 @@ static CMPIStatus pool_to_rasd(const CMP CMPIStatus s = {CMPI_RC_OK, NULL}; const char *poolid; uint16_t type; + CMPIInstance *inst; if (!match_hypervisor_prefix(ref, info)) return s; @@ -202,6 +203,9 @@ static CMPIStatus pool_to_rasd(const CMP goto out; } + s = get_pool_inst(_BROKER, ref, &inst); + if ((s.rc != CMPI_RC_OK) || (inst == NULL)) + goto out; rasds_from_pool(type, ref,
Kaitlin Rupert wrote:
Updates from last set: -"Cleanup device_type_from_poolid() in DevicePool" - reverted the function name back to device_type_from_poolid() instead of device_type_from_str()
This cleanup patch doesn't really fit in with this set. I can send it separately if need be.
-Added two new patches: -"GetInstance() in DevicePool returns an instance for invalid refs." -"Roll DevicePool GetInstance() functionality into a seperate function."
I hope adding a new function in this patch doesn't make things too messy. Virt_DevicePool has a lot of helper functions. Once this bug / issue is resolved, I can look into whether any cleanup needs to be done here.
-Re-worked EAFP and RAFP patches based on the two new patches.
This fix makes it so that EAFP and RAFP do not return instances if an invalid ref is given.
These patches are dependant on the "Clean up RAFP and EAFP pool_to_* handlers." patchset.
-- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Looks like a good set to me. The one patch is a little bit incongruous, but the total volume is quite manageable so it's not really an issue. Also, having a patch that may or may not fit into a set is far less of a concern to me than having changes that may or may not belong in a patch. Each individual patch is clear and consistent, so don't worry. Also, the (inst == NULL) check is a good catch. I wouldn't be surprised if we need that in a few more spots, especially since we've been a bit inconsistent when it comes to the "Is a NULL instance an error or just a NULL instance?" question. -- -Jay
Jay Gagnon wrote:
Looks like a good set to me. The one patch is a little bit incongruous, but the total volume is quite manageable so it's not really an issue. Also, having a patch that may or may not fit into a set is far less of a concern to me than having changes that may or may not belong in a patch. Each individual patch is clear and consistent, so don't worry.
Sounds good to me. Thanks =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
participants (2)
-
Jay Gagnon -
Kaitlin Rupert