[PATCH 0 of 3] EAFP and RAFP support invalid instances.
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 1198183661 28800 # Node ID 912a37bdd53f3fbf8e13026d8cbc02921aa028c1 # Parent 84eb3549e19956d912684a09d58631211724a720 Cleanup device_type_from_poolid() in DevicePool. Change device_type_from_poolid() to device_type_from_str() so that this function can be used for DevicePool classnames as well as InstanceIDs. 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 912a37bdd53f src/Virt_DevicePool.c --- a/src/Virt_DevicePool.c Thu Dec 20 10:14:00 2007 -0800 +++ b/src/Virt_DevicePool.c Thu Dec 20 12:47:41 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_str(const char *str) +{ + if (STARTS_WITH(str, "NetworkPool")) return VIRT_DEV_NET; - else if (strstr(id, "DiskPool")) + else if (STARTS_WITH(str, "DiskPool")) return VIRT_DEV_DISK; - else if (strstr(id, "Memory")) + else if (STARTS_WITH(str, "MemoryPool")) return VIRT_DEV_MEM; - else if (strstr(id, "Processor")) + else if (STARTS_WITH(str, "ProcessorPool")) return VIRT_DEV_VCPU; else return VIRT_DEV_UNKNOWN; diff -r 84eb3549e199 -r 912a37bdd53f src/Virt_DevicePool.h --- a/src/Virt_DevicePool.h Thu Dec 20 10:14:00 2007 -0800 +++ b/src/Virt_DevicePool.h Thu Dec 20 12:47:41 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_str(const char *str); /** * Get all device pools on the system for the given connection
KR> Change device_type_from_poolid() to device_type_from_str() so that KR> this function can be used for DevicePool classnames as well as KR> InstanceIDs. How about device_type_from_id()? The "from_str" sounds like instance parsing or something like that. ID could mean DeviceID or InstanceID. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
KR> Change device_type_from_poolid() to device_type_from_str() so that KR> this function can be used for DevicePool classnames as well as KR> InstanceIDs.
How about device_type_from_id()? The "from_str" sounds like instance parsing or something like that. ID could mean DeviceID or InstanceID.
The change to "from_str()" here is so that one would be able to pass the classname in. The logic for determining the device / pool type is the same for the InstanceID and the classname. But if this is confusing, I could change the name to something else. Or, I could add 2 wrapper functions with clearer names. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
KR> The change to "from_str()" here is so that one would be able to KR> pass the classname in. The logic for determining the device / KR> pool type is the same for the InstanceID and the classname. Right, but we could potentially need to look at more of the ID in some cases to determine the type, right? If we had disk pools that represent LVMs separately from file images or something like that. I suppose it's reasonable to think that the classname would always indicate the subtype, but it just seems cleaner to me to always do it From an ID. We can adjust it later if we come across this situation, I just thought it was worth considering. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
KR> The change to "from_str()" here is so that one would be able to KR> pass the classname in. The logic for determining the device / KR> pool type is the same for the InstanceID and the classname.
Right, but we could potentially need to look at more of the ID in some cases to determine the type, right? If we had disk pools that represent LVMs separately from file images or something like that. I suppose it's reasonable to think that the classname would always indicate the subtype, but it just seems cleaner to me to always do it From an ID.
Ah, yes - this is a really good point. Hmm.. cu_compare_ref() seems like it could be used , except that the handler would need to construct an instance just for the purpose of comparing it against the ref, which seems a little unnecessary to me. It's probably best to leave device_type_from_poolid() as is, but devise some other way to further make sure the ref is a valid one. Ideally, GetInstance() from Virt_DevicePool should be able to check whether the ref is valid. However GetInstance() also supports refs with mis-matching InstanceIDs and class types. =) So, if this issue is fixed in Virt_DevicePool, the same logic / function could be reused in RAFP and EAFP. I'll see if I can cook up something a little more elegant in Virt_DevicePool. Thanks for the feedback! =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198183662 28800 # Node ID 3a2bf29ad7376918fdfd77990cbdc15c8d9bf4c7 # Parent 912a37bdd53f3fbf8e13026d8cbc02921aa028c1 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). Now that device_type_from_str() allows strings, instead of passing the poolid, pass the classname base. If the InstanceID doesn't match that of a valid pool, devs_from_pool() will return an empty instance list. 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 912a37bdd53f -r 3a2bf29ad737 src/Virt_ElementAllocatedFromPool.c --- a/src/Virt_ElementAllocatedFromPool.c Thu Dec 20 12:47:41 2007 -0800 +++ b/src/Virt_ElementAllocatedFromPool.c Thu Dec 20 12:47:42 2007 -0800 @@ -215,7 +215,7 @@ static CMPIStatus pool_to_vdev(const CMP CU_DEBUG("Got %s\n", poolid); - type = device_type_from_poolid(poolid); + type = device_type_from_str(class_base_name(CLASSNAME(ref))); if (type == VIRT_DEV_UNKNOWN) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED,
KR> - type = device_type_from_poolid(poolid); KR> + type = device_type_from_str(class_base_name(CLASSNAME(ref))); A couple things here. First, I think that we should be passing in the id, in case we need to do more specific checking in the future. We can check the classname of the ref separately. Second, class_base_name() returns a strdup()'d string, so you leak that here. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
KR> - type = device_type_from_poolid(poolid); KR> + type = device_type_from_str(class_base_name(CLASSNAME(ref)));
A couple things here. First, I think that we should be passing in the id, in case we need to do more specific checking in the future. We can check the classname of the ref separately.
We'll need to do the classname checking here because the bug is that you can pass a Xen_MemoryPool class with "ProcessorPool/0" as the ID, which isn't valid.
Second, class_base_name() returns a strdup()'d string, so you leak that here.
Oh yes, good catch. Thanks. =) I'll re-work this. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
KR> We'll need to do the classname checking here because the bug is KR> that you can pass a Xen_MemoryPool class with "ProcessorPool/0" as KR> the ID, which isn't valid. Right. Maybe cu_compare_ref() be used to do this here? -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1198183692 28800 # Node ID 89eacde64e5385e9003a8312983ab41468d8f54b # Parent 3a2bf29ad7376918fdfd77990cbdc15c8d9bf4c7 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). Now that device_type_from_str() allows strings, instead of passing the poolid, pass the classname base. If the InstanceID doesn't match that of a valid pool, rasds_from_pool() will return an empty instance list. 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 3a2bf29ad737 -r 89eacde64e53 src/Virt_ResourceAllocationFromPool.c --- a/src/Virt_ResourceAllocationFromPool.c Thu Dec 20 12:47:42 2007 -0800 +++ b/src/Virt_ResourceAllocationFromPool.c Thu Dec 20 12:48:12 2007 -0800 @@ -194,7 +194,7 @@ static CMPIStatus pool_to_rasd(const CMP goto out; } - type = device_type_from_poolid(poolid); + type = device_type_from_str(class_base_name(CLASSNAME(ref))); if (type == VIRT_DEV_UNKNOWN) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED,
participants (2)
-
Dan Smith -
Kaitlin Rupert