1 p.m.
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.
1 p.m.
New subject: [PATCH 1 of 5] Cleanup device_type_from_poolid() in DevicePool
# HG changeset patch
# User Kaitlin Rupert <karupert(a)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(a)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
1 p.m.
New subject: [PATCH 2 of 5] GetInstance() in DevicePool returns an instance for invalid refs
# HG changeset patch
# User Kaitlin Rupert <karupert(a)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(a)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 {
8:48 a.m.
New subject: [PATCH 2 of 5] GetInstance() in DevicePool returns an instance for invalid refs
Kaitlin Rupert wrote:
# HG changeset patch
# User Kaitlin Rupert <karupert(a)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(a)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
9:26 a.m.
New subject: [PATCH 2 of 5] GetInstance() in DevicePool returns an instance for invalid refs
Jay Gagnon wrote:
Kaitlin Rupert wrote:
> # HG changeset patch
> # User Kaitlin Rupert <karupert(a)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(a)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(a)us.ibm.com
1 p.m.
New subject: [PATCH 3 of 5] Roll DevicePool GetInstance() functionality into a seperate function
# HG changeset patch
# User Kaitlin Rupert <karupert(a)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(a)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
/*
1 p.m.
New subject: [PATCH 4 of 5] Add check in EAFP to ensure ref passed is an actual instance
# HG changeset patch
# User Kaitlin Rupert <karupert(a)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(a)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);
1 p.m.
New subject: [PATCH 5 of 5] Add check in RAFP to ensure ref passed is an actual instance
# HG changeset patch
# User Kaitlin Rupert <karupert(a)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(a)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,
1:15 p.m.
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(a)us.ibm.com
9:09 a.m.
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
9:28 a.m.
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(a)us.ibm.com
6132
days inactive
6145
days old
10 comments
2 participants
participants (2)
-
Jay Gagnon -
Kaitlin Rupert