1:21 p.m.
To enable the check for the client's given object path in ElementCapabilities, changes
to the ComputerSystem and AllocationCapabilities provider have been necessary.
1:21 p.m.
New subject: [PATCH 1 of 3] Add function to validate the client given object path o ComputerSystem
# HG changeset patch
# User Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
# Date 1199971251 -3600
# Node ID a45042de9fd40ef1cb150c854f506baf03690ed5
# Parent b69727ddc9a45a32c75547a90364759500280398
Add function to validate the client given object path o ComputerSystem
Signed-off-by: Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
diff -r b69727ddc9a4 -r a45042de9fd4 src/Virt_ComputerSystem.c
--- a/src/Virt_ComputerSystem.c Thu Jan 10 11:04:39 2008 +0100
+++ b/src/Virt_ComputerSystem.c Thu Jan 10 14:20:51 2008 +0100
@@ -285,7 +285,7 @@ CMPIInstance *instance_from_name(const C
dom = virDomainLookupByName(conn, name);
if (dom == NULL)
- return 0;
+ return NULL;
instance = get_typed_instance(broker,
pfx_from_conn(conn),
@@ -374,45 +374,62 @@ static CMPIStatus return_enum_domains(co
return s;
}
-static CMPIStatus get_domain(const CMPIObjectPath *reference,
- const CMPIResult *results,
- const char *name)
-{
- CMPIInstance *inst;
- CMPIStatus s;
+static CMPIStatus get_domain(const CMPIBroker *broker,
+ const CMPIObjectPath *reference,
+ CMPIInstance **inst)
+{
+ CMPIInstance *_inst;
+ CMPIStatus s = {CMPI_RC_OK, NULL};
virConnectPtr conn = NULL;
- const char *prop = NULL;
-
- if (!provider_is_responsible(_BROKER, reference, &s)) {
- CMSetStatus(&s, CMPI_RC_ERR_NOT_FOUND);
+ const char *name;
+
+ if (!provider_is_responsible(broker, reference, &s))
return s;
- }
-
- conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s);
+
+ if (cu_get_str_path(reference, "Name", &name) != CMPI_RC_OK) {
+ cu_statusf(broker, &s,
+ CMPI_RC_ERR_FAILED,
+ "No domain name specified");
+ return s;
+ }
+
+ conn = connect_by_classname(broker, CLASSNAME(reference), &s);
if (conn == NULL)
return s;
- inst = instance_from_name(_BROKER, conn, name, reference);
- if (inst == NULL) {
- cu_statusf(_BROKER, &s,
- CMPI_RC_ERR_FAILED,
- "Unable to find `%s'", name);
- goto out;
- }
-
- prop = cu_compare_ref(reference, inst);
+ _inst = instance_from_name(broker, conn, name, reference);
+ if (_inst == NULL) {
+ cu_statusf(broker, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", name);
+ goto out;
+ }
+
+ out:
+ virConnectClose(conn);
+ *inst = _inst;
+
+ return s;
+}
+
+CMPIStatus validate_domain_ref(const CMPIBroker *broker,
+ const CMPIObjectPath *ref)
+{
+ CMPIStatus s = {CMPI_RC_OK, NULL};
+ CMPIInstance *inst;
+ const char *prop;
+
+ s = get_domain(broker, ref, &inst);
+ if (s.rc != CMPI_RC_OK)
+ return s;
+
+ prop = cu_compare_ref(ref, inst);
if (prop != NULL) {
- cu_statusf(_BROKER, &s,
+ cu_statusf(broker, &s,
CMPI_RC_ERR_NOT_FOUND,
"No such instance (%s)", prop);
- goto out;
- }
-
- CMReturnInstance(results, inst);
- CMSetStatus(&s, CMPI_RC_OK);
- out:
- virConnectClose(conn);
-
+ }
+
return s;
}
@@ -440,19 +457,25 @@ static CMPIStatus GetInstance(CMPIInstan
const CMPIObjectPath *reference,
const char **properties)
{
- const char *name;
-
- if (cu_get_str_path(reference, "Name", &name) != CMPI_RC_OK) {
- CMPIStatus s;
-
- cu_statusf(_BROKER, &s,
- CMPI_RC_ERR_FAILED,
- "No domain name specified");
-
+ CMPIStatus s = {CMPI_RC_OK, NULL};
+ CMPIInstance *inst;
+ const char *prop = NULL;
+
+ s = get_domain(_BROKER, reference, &inst);
+ if (s.rc != CMPI_RC_OK)
return s;
- }
-
- return get_domain(reference, results, name);
+
+ prop = cu_compare_ref(reference, inst);
+ if (prop != NULL) {
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", prop);
+ return s;
+ }
+
+ CMReturnInstance(results, inst);
+
+ return s;
}
DEFAULT_CI();
diff -r b69727ddc9a4 -r a45042de9fd4 src/Virt_ComputerSystem.h
--- a/src/Virt_ComputerSystem.h Thu Jan 10 11:04:39 2008 +0100
+++ b/src/Virt_ComputerSystem.h Thu Jan 10 14:20:51 2008 +0100
@@ -52,6 +52,16 @@ int enum_domains(const CMPIBroker *broke
const char *ns,
struct inst_list *instlist);
+/**
+ * Validate the client given domain object path
+ *
+ * @param broker A pointer to the current broker
+ * @param ref The client given object path
+ * @returns CMPIStatus
+ */
+CMPIStatus validate_domain_ref(const CMPIBroker *broker,
+ const CMPIObjectPath *ref);
+
#endif
7:14 p.m.
New subject: [PATCH 1 of 3] Add function to validate the client given object path o ComputerSystem
Heidi Eckhart wrote:
I like the idea of a validate function. But the body of GetInstance()
is almost identical to the body of validate_domain_ref() - the only
exception is that one returns an instance and the other returns a status.
Could these functions be consolidated? You could change get_domain() to
something like _get_domain(). Then get_domain() could call
_get_domain() and validate_domain_ref(). When you need the instance
returned, you can use get_domain(), otherwise you can just call
validate_domain_ref() directly to validate.
-static CMPIStatus get_domain(const CMPIObjectPath *reference,
- const CMPIResult *results,
- const char *name)
-{
- CMPIInstance *inst;
- CMPIStatus s;
+static CMPIStatus get_domain(const CMPIBroker *broker,
+ const CMPIObjectPath *reference,
+ CMPIInstance **inst)
+{
+ CMPIInstance *_inst;
+ CMPIStatus s = {CMPI_RC_OK, NULL};
virConnectPtr conn = NULL;
- const char *prop = NULL;
-
- if (!provider_is_responsible(_BROKER, reference, &s)) {
- CMSetStatus(&s, CMPI_RC_ERR_NOT_FOUND);
+ const char *name;
+
+ if (!provider_is_responsible(broker, reference, &s))
return s;
- }
-
- conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s);
+
+ if (cu_get_str_path(reference, "Name", &name) != CMPI_RC_OK) {
+ cu_statusf(broker, &s,
+ CMPI_RC_ERR_FAILED,
+ "No domain name specified");
+ return s;
+ }
+
+ conn = connect_by_classname(broker, CLASSNAME(reference), &s);
if (conn == NULL)
return s;
- inst = instance_from_name(_BROKER, conn, name, reference);
- if (inst == NULL) {
- cu_statusf(_BROKER, &s,
- CMPI_RC_ERR_FAILED,
- "Unable to find `%s'", name);
- goto out;
- }
-
- prop = cu_compare_ref(reference, inst);
+ _inst = instance_from_name(broker, conn, name, reference);
+ if (_inst == NULL) {
+ cu_statusf(broker, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", name);
+ goto out;
+ }
+
+ out:
+ virConnectClose(conn);
+ *inst = _inst;
+
+ return s;
+}
+
+CMPIStatus validate_domain_ref(const CMPIBroker *broker,
+ const CMPIObjectPath *ref)
+{
+ CMPIStatus s = {CMPI_RC_OK, NULL};
+ CMPIInstance *inst;
+ const char *prop;
+
+ s = get_domain(broker, ref, &inst);
+ if (s.rc != CMPI_RC_OK)
+ return s;
+
+ prop = cu_compare_ref(ref, inst);
if (prop != NULL) {
- cu_statusf(_BROKER, &s,
+ cu_statusf(broker, &s,
CMPI_RC_ERR_NOT_FOUND,
"No such instance (%s)", prop);
- goto out;
- }
-
- CMReturnInstance(results, inst);
- CMSetStatus(&s, CMPI_RC_OK);
- out:
- virConnectClose(conn);
-
+ }
+
return s;
}
@@ -440,19 +457,25 @@ static CMPIStatus GetInstance(CMPIInstan
const CMPIObjectPath *reference,
const char **properties)
{
- const char *name;
-
- if (cu_get_str_path(reference, "Name", &name) != CMPI_RC_OK) {
- CMPIStatus s;
-
- cu_statusf(_BROKER, &s,
- CMPI_RC_ERR_FAILED,
- "No domain name specified");
-
+ CMPIStatus s = {CMPI_RC_OK, NULL};
+ CMPIInstance *inst;
+ const char *prop = NULL;
+
+ s = get_domain(_BROKER, reference, &inst);
+ if (s.rc != CMPI_RC_OK)
return s;
- }
-
- return get_domain(reference, results, name);
+
+ prop = cu_compare_ref(reference, inst);
+ if (prop != NULL) {
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", prop);
+ return s;
+ }
+
+ CMReturnInstance(results, inst);
+
+ return s;
}
--
Kaitlin Rupert
IBM Linux Technology Center
karupert(a)us.ibm.com
11:11 a.m.
New subject: [PATCH 1 of 3] Add function to validate the client given object path o ComputerSystem
Kaitlin Rupert wrote:
Heidi Eckhart wrote:
I like the idea of a validate function. But the body of GetInstance()
is almost identical to the body of validate_domain_ref() - the only
exception is that one returns an instance and the other returns a status.
Could these functions be consolidated?
You could change get_domain() to something like _get_domain(). Then
get_domain() could call _get_domain() and validate_domain_ref(). When
you need the instance returned, you can use get_domain(), otherwise
you can just call validate_domain_ref() directly to validate.
In general a very good idea, but it causes a "hen and egg" situation.
The intention of validate_domain_ref() is to be used by other providers
to check if the client given object path is valid for this system. That
means validate_domain_ref() can only get the object path as input
parameter (besides the broker), because it should not be the calling
function's responsibility to retrieve the domain instance before using
validate_domain_ref(). This means validate_domain_ref() has to retrieve
the real instance and compare this one against the client given object path.
If the functions are reordered like above, this causes that get_domain()
calls _get_domain() twice. First to retrieve the instance for a later
returning - and second within validate_domain_ref() as it needs the
instance for the comparison. So validate_domain_ref() can not be used
internally. Its only an interface for external provider usage.
But your comments made me rethink about the patch. So I've consolidated
and reordered a bit.
- added a new function to libcmpiutil - cu_validate_ref(ref, inst) -
that does the check between the system instance and the client given
instance
- get_domain() is now using the new libcmpiutil function
- the parameter CMPIInstance **inst is now used to "configure"
get_domain(); for internal usage the inst is returned and for external
usage this can be ignored by setting it to NULL
- validate_domain_ref() is now only an interface name for the client;
this could also be removed and the provider has then to call
get_domain(broker, ref, NULL) or simply ignores the returned instance;
but what I do not really like is, that this can confuse the reader of
the code; but I'm open for discussion and opinions
Will send out the patch soon.
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
5:41 p.m.
New subject: [PATCH 1 of 3] Add function to validate the client given object path o ComputerSystem
Heidi Eckhart wrote:.
But your comments made me rethink about the patch. So I've consolidated
and reordered a bit.
- added a new function to libcmpiutil - cu_validate_ref(ref, inst) -
that does the check between the system instance and the client given
instance
I think this is a good idea. It seems like it'll be useful in other
places as well.
- get_domain() is now using the new libcmpiutil function
- the parameter CMPIInstance **inst is now used to "configure"
get_domain(); for internal usage the inst is returned and for external
usage this can be ignored by setting it to NULL
- validate_domain_ref() is now only an interface name for the client;
this could also be removed and the provider has then to call
get_domain(broker, ref, NULL) or simply ignores the returned instance;
but what I do not really like is, that this can confuse the reader of
the code; but I'm open for discussion and opinions
As far as readability, I'm fine with this method. As an alternative,
you could have get_domain() return an instance and take a CMPIStatus
variable as a parameter. The provider can then choose to ignore the
returned instance. But that doesn't really fit with our existing code
style.
--
Kaitlin Rupert
IBM Linux Technology Center
karupert(a)us.ibm.com
3:11 p.m.
New subject: [PATCH 1 of 3] Add function to validate the client given object path o ComputerSystem
Kaitlin Rupert wrote:
Heidi Eckhart wrote:.
>
> But your comments made me rethink about the patch. So I've
> consolidated and reordered a bit.
> - added a new function to libcmpiutil - cu_validate_ref(ref, inst) -
> that does the check between the system instance and the client given
> instance
I think this is a good idea. It seems like it'll be useful in other
places as well.
> - get_domain() is now using the new libcmpiutil function
> - the parameter CMPIInstance **inst is now used to "configure"
> get_domain(); for internal usage the inst is returned and for
> external usage this can be ignored by setting it to NULL
> - validate_domain_ref() is now only an interface name for the client;
> this could also be removed and the provider has then to call
> get_domain(broker, ref, NULL) or simply ignores the returned
> instance; but what I do not really like is, that this can confuse the
> reader of the code; but I'm open for discussion and opinions
As far as readability, I'm fine with this method. As an alternative,
you could have get_domain() return an instance and take a CMPIStatus
variable as a parameter. The provider can then choose to ignore the
returned instance.
Ok, then it might not be that big issue to simply ignore the
returned
instance in case of success.
But that doesn't really fit with our existing code style.
If we leave get_domain() as it is now and only make it external, this
would follow the coding style of e.g. DevicePool - get_pool_inst(...) -
and EnabledLogicalElementCapabilities - get_ele_cap(...). So removing
validate_domain_ref() and making get_domain() externally available is a
good compromise.
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
1:21 p.m.
New subject: [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC
# HG changeset patch
# User Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
# Date 1199971252 -3600
# Node ID fd476f08b35e235ba7e50d326e5602018ebc3730
# Parent a45042de9fd40ef1cb150c854f506baf03690ed5
Make alloc_cap_instances() available for external use by EC
The association provider ElementCapabilities needs the
alloc_cap_instances() function to access to the list of
available AllocationCapabilities. To return this instance
to the association provider a CMPIInstance** is added to
the parameter list.
Signed-off-by: Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
diff -r a45042de9fd4 -r fd476f08b35e src/Virt_AllocationCapabilities.c
--- a/src/Virt_AllocationCapabilities.c Thu Jan 10 14:20:51 2008 +0100
+++ b/src/Virt_AllocationCapabilities.c Thu Jan 10 14:20:52 2008 +0100
@@ -31,6 +31,7 @@
#include "misc_util.h"
+#include "Virt_AllocationCapabilities.h"
#include "Virt_DevicePool.h"
const static CMPIBroker *_BROKER;
@@ -71,12 +72,13 @@ static CMPIStatus ac_from_pool(const CMP
return s;
}
-static CMPIStatus alloc_cap_instances(const CMPIBroker *broker,
- const CMPIObjectPath *ref,
- const CMPIResult *results,
- bool names_only,
- const char **properties,
- const char *id)
+CMPIStatus alloc_cap_instances(const CMPIBroker *broker,
+ const CMPIObjectPath *ref,
+ const CMPIResult *results,
+ bool names_only,
+ const char **properties,
+ const char *id,
+ CMPIInstance **inst)
{
int i;
virConnectPtr conn = NULL;
@@ -86,21 +88,19 @@ static CMPIStatus alloc_cap_instances(co
CMPIStatus s = {CMPI_RC_OK, NULL};
const char *inst_id;
- CU_DEBUG("In alloc_cap_instances()");
+ if (!provider_is_responsible(broker, ref, &s))
+ goto out;
+
+ conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s);
+ if (conn == NULL) {
+ cu_statusf(broker, &s,
+ CMPI_RC_ERR_FAILED,
+ "Could not connect to hypervisor");
+ goto out;
+ }
inst_list_init(&device_pool_list);
inst_list_init(&alloc_cap_list);
-
- if (!provider_is_responsible(broker, ref, &s))
- goto out;
-
- conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s);
- if (conn == NULL) {
- cu_statusf(broker, &s,
- CMPI_RC_ERR_FAILED,
- "Could not connect to hypervisor");
- goto out;
- }
s = get_all_pools(broker, conn, NAMESPACE(ref), &device_pool_list);
if (s.rc != CMPI_RC_OK) {
@@ -138,19 +138,25 @@ static CMPIStatus alloc_cap_instances(co
if (id && !inst_id) {
cu_statusf(broker, &s,
CMPI_RC_ERR_NOT_FOUND,
- "Requested Object could not be found.");
+ "No such instance (%s)", id);
goto out;
}
- if (names_only)
- cu_return_instance_names(results, &alloc_cap_list);
- else
- cu_return_instances(results, &alloc_cap_list);
+ if (results) {
+ if (names_only)
+ cu_return_instance_names(results, &alloc_cap_list);
+ else
+ cu_return_instances(results, &alloc_cap_list);
+ } else {
+ if (inst)
+ *inst = alloc_cap_list.list[0];
+ }
out:
virConnectClose(conn);
inst_list_free(&alloc_cap_list);
inst_list_free(&device_pool_list);
+
return s;
}
@@ -175,7 +181,8 @@ static CMPIStatus GetInstance(CMPIInstan
results,
false,
properties,
- id);
+ id,
+ NULL);
}
static CMPIStatus EnumInstanceNames(CMPIInstanceMI *self,
@@ -188,6 +195,7 @@ static CMPIStatus EnumInstanceNames(CMPI
results,
true,
NULL,
+ NULL,
NULL);
}
@@ -202,6 +210,7 @@ static CMPIStatus EnumInstances(CMPIInst
results,
false,
properties,
+ NULL,
NULL);
}
diff -r a45042de9fd4 -r fd476f08b35e src/Virt_AllocationCapabilities.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/Virt_AllocationCapabilities.h Thu Jan 10 14:20:52 2008 +0100
@@ -0,0 +1,57 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * Authors:
+ * Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+#ifndef __VIRT_ALLOCATIONCAPABILITIES_H
+#define __VIRT_ALLOCATIONCAPABILITIES_H
+
+#include "misc_util.h"
+
+/**
+ * Return the instance of the AllocationCapabilities instance,
+ * defined by the id
+ *
+ * @param broker A pointer to the current broker
+ * @param ref The reference
+ * @param results The results pointer used to return results to the broker
+ * @param names_only True returns object pathes, false returns instances
+ * @param properties list of properties to set
+ * @param id The InstanceID of the AllocationCapabilities
+ * @param inst Contains the instance pointer in case of success
+ * @returns The status of this operation
+ */
+CMPIStatus alloc_cap_instances(const CMPIBroker *broker,
+ const CMPIObjectPath *ref,
+ const CMPIResult *results,
+ bool names_only,
+ const char **properties,
+ const char *id,
+ CMPIInstance **inst);
+
+#endif
+
+/*
+ * Local Variables:
+ * mode: C
+ * c-set-style: "K&R"
+ * tab-width: 8
+ * c-basic-offset: 8
+ * indent-tabs-mode: nil
+ * End:
+ */
8:01 p.m.
New subject: [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC
HE> + if (!provider_is_responsible(broker, ref, &s))
HE> + goto out;
HE> +
HE> + conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s);
HE> + if (conn == NULL) {
HE> + cu_statusf(broker, &s,
HE> + CMPI_RC_ERR_FAILED,
HE> + "Could not connect to hypervisor");
HE> + goto out;
HE> + }
HE> inst_list_init(&device_pool_list);
HE> inst_list_init(&alloc_cap_list);
HE> -
HE> - if (!provider_is_responsible(broker, ref, &s))
HE> - goto out;
HE> -
HE> - conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s);
HE> - if (conn == NULL) {
HE> - cu_statusf(broker, &s,
HE> - CMPI_RC_ERR_FAILED,
HE> - "Could not connect to hypervisor");
HE> - goto out;
HE> - }
Moving these functions above the inst_list_init() calls will cause a
crash on error, because the out target tries to free them.
As far as I can tell, these aren't related to the rest of the patch in
any way. If there is something to fix here, perhaps we can call it
out and make it a distinct patch?
HE> @@ -138,19 +138,25 @@ static CMPIStatus alloc_cap_instances(co
HE> if (id && !inst_id) {
HE> cu_statusf(broker, &s,
HE> CMPI_RC_ERR_NOT_FOUND,
HE> - "Requested Object could not be found.");
HE> + "No such instance (%s)", id);
Also, this change doesn't seem to be necessary, and makes this one
error message inconsistent with many others that we already have. If
this is a personal preference change, then it should be a distinct
patch that either changes (or plans to change) the other instances of
these error messages.
HE> - if (names_only)
HE> - cu_return_instance_names(results, &alloc_cap_list);
HE> - else
HE> - cu_return_instances(results, &alloc_cap_list);
HE> + if (results) {
HE> + if (names_only)
HE> + cu_return_instance_names(results, &alloc_cap_list);
HE> + else
HE> + cu_return_instances(results, &alloc_cap_list);
HE> + } else {
HE> + if (inst)
HE> + *inst = alloc_cap_list.list[0];
HE> + }
I'm not okay with this change. From what I can see, this function was
modified to have a dual return path, where either results is passed
in, and inst is NULL, or vice versa. If you want to have the
opportunity to intercept the instances coming back, then the function
should take in an inst_list, and populate it. The caller can then
either return or inspect the result.
Other places in the code, we have a function as described, and then a
pass-through function called "return_foo()" that calls the function
that returns the list, and then calls cm_return_instance...(). Such a
function seems appropriate here.
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
12:06 p.m.
New subject: [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC
Dan Smith wrote:
Moving these functions above the inst_list_init() calls will cause a
crash on error, because the out target tries to free them.
Mhh, as we have many free operations now that can handle NULL pointers,
I suggest to also make inst_list_free() able to handle NULL pointers.
As far as I can tell, these aren't related to the rest of the
patch in
any way. If there is something to fix here, perhaps we can call it
out and make it a distinct patch?
Agreed. I will cook up a patch for inst_list_free(). The intention to
move the inst_list_init() functions below connect_by_classname() was to
avoid some cycles, because the case where the provider exits right after
provider_is_responsible() or connect_by_classname() can happen often.
HE> @@ -138,19 +138,25 @@ static CMPIStatus
alloc_cap_instances(co
HE> if (id && !inst_id) {
HE> cu_statusf(broker, &s,
HE> CMPI_RC_ERR_NOT_FOUND,
HE> - "Requested Object could not be found.");
HE> + "No such instance (%s)", id);
Also, this change doesn't seem to be necessary, and makes this one
error message inconsistent with many others that we already have. If
this is a personal preference change, then it should be a distinct
patch that either changes (or plans to change) the other instances of
these error messages.
Agreed. I saw that we already have a task for "Fix up all the error
codes in returned status values (everything is currently
CMPI_RC_ERR_FAILED)". So we should leave this discussion for there. I
will remove this change.
HE> - if (names_only)
HE> - cu_return_instance_names(results, &alloc_cap_list);
HE> - else
HE> - cu_return_instances(results, &alloc_cap_list);
HE> + if (results) {
HE> + if (names_only)
HE> + cu_return_instance_names(results, &alloc_cap_list);
HE> + else
HE> + cu_return_instances(results, &alloc_cap_list);
HE> + } else {
HE> + if (inst)
HE> + *inst = alloc_cap_list.list[0];
HE> + }
I'm not okay with this change. From what I can see, this function was
modified to have a dual return path, where either results is passed
in, and inst is NULL, or vice versa. If you want to have the
opportunity to intercept the instances coming back, then the function
should take in an inst_list, and populate it. The caller can then
either return or inspect the result.
Very good catch. Sometimes the goal to make the best out of the existing
code makes oneself blind for bigger changes. Thanks.
Other places in the code, we have a function as described, and then
a
pass-through function called "return_foo()" that calls the function
that returns the list, and then calls cm_return_instance...(). Such a
function seems appropriate here.
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
4:10 p.m.
New subject: [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC
HE> Mhh, as we have many free operations now that can handle NULL
HE> pointers, I suggest to also make inst_list_free() able to handle
HE> NULL pointers.
But they're not NULL pointers at that point, the list.list is a
garbage pointer. Unless you define them as:
struct inst_list list = {NULL, 0, 0};
Otherwise, the pointer will be garbage and a free() will smash the
heap.
The inst_list_init() call doesn't allocate any memory, it just
initializes the contents of the struct, so why not always do that
before proceeding?
HE> Agreed. I will cook up a patch for inst_list_free(). The intention
HE> to move the inst_list_init() functions below
HE> connect_by_classname() was to avoid some cycles, because the case
HE> where the provider exits right after provider_is_responsible() or
HE> connect_by_classname() can happen often.
I think this optimization is likely lost in the noise of the roaring
CIMOM above us, so I'm not sure it's all that important. However, a
macro could help make this cleaner I think. Something like what they
do in the kernel might be helpful:
#define DECLARE_INST_LIST(x) struct inst_list x = {NULL, 0, 0}
so that we can just do this in a function to have pre-initialized
lists and avoid the inst_list_init() calls:
int function_foo(...) {
int a;
char b;
DECLARE_INST_LIST(list_one);
DECLARE_INST_LIST(list_two);
...
}
I still question the value of this optimization, as I think the
compiler will inline the inst_list_init() function. At that point,
this:
struct inst_list foo = {NULL, 0, 0};
surely becomes the same code as this:
struct inst_list foo;
foo.list = NULL;
foo.max = foo.cur = 0;
Anyone else have thoughts on this?
HE> Agreed. I saw that we already have a task for "Fix up all the
HE> error codes in returned status values (everything is currently
HE> CMPI_RC_ERR_FAILED)". So we should leave this discussion for
HE> there. I will remove this change.
Yes, we definitely have work to do on our return codes and error
messages, but I'd rather address them as a group than sprinkle them in
with other changes :)
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
1:13 p.m.
New subject: [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC
Dan Smith wrote:
HE> Mhh, as we have many free operations now that can handle NULL
HE> pointers, I suggest to also make inst_list_free() able to handle
HE> NULL pointers.
But they're not NULL pointers at that point, the list.list is a
garbage pointer. Unless you define them as:
struct inst_list list = {NULL, 0, 0};
Otherwise, the pointer will be garbage and a free() will smash the
heap.
The inst_list_init() call doesn't allocate any memory, it just
initializes the contents of the struct, so why not always do that
before proceeding?
Ok, thank you for this clarification.
HE> Agreed. I will cook up a patch for inst_list_free(). The
intention
HE> to move the inst_list_init() functions below
HE> connect_by_classname() was to avoid some cycles, because the case
HE> where the provider exits right after provider_is_responsible() or
HE> connect_by_classname() can happen often.
I think this optimization is likely lost in the noise of the roaring
CIMOM above us, so I'm not sure it's all that important. However, a
macro could help make this cleaner I think. Something like what they
do in the kernel might be helpful:
#define DECLARE_INST_LIST(x) struct inst_list x = {NULL, 0, 0}
so that we can just do this in a function to have pre-initialized
lists and avoid the inst_list_init() calls:
int function_foo(...) {
int a;
char b;
DECLARE_INST_LIST(list_one);
DECLARE_INST_LIST(list_two);
...
}
I still question the value of this optimization, as I think the
compiler will inline the inst_list_init() function. At that point,
this:
struct inst_list foo = {NULL, 0, 0};
surely becomes the same code as this:
struct inst_list foo;
foo.list = NULL;
foo.max = foo.cur = 0;
Anyone else have thoughts on this?
Your explanation is absolutely clear and shows me, that I was nitpickier
than a nitpicker ;). I can only agree that the "optimization" will get
"lost in the noise of the roaring CIMOM above us". I like the suggestion
of the DECLARE macro, but understand, that we have more important things
to do and fix. So skipping it.
HE> Agreed. I saw that we already have a task for "Fix up all
the
HE> error codes in returned status values (everything is currently
HE> CMPI_RC_ERR_FAILED)". So we should leave this discussion for
HE> there. I will remove this change.
Yes, we definitely have work to do on our return codes and error
messages, but I'd rather address them as a group than sprinkle them in
with other changes :)
Thanks!
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
1:21 p.m.
New subject: [PATCH 3 of 3] EC: Validation of client given object path missing
# HG changeset patch
# User Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
# Date 1199971253 -3600
# Node ID d4471d3ac4766af68f6bf151813f0ec246b0e409
# Parent fd476f08b35e235ba7e50d326e5602018ebc3730
EC: Validation of client given object path missing
The validation of the client's given ResourcePool object
path is missing. Instead the submitted InstanceID is
taken as value for the AllocationCapabilities InstanceID.
wbemain -ac CIM_ElementCapabilities
'http://localhost/root/virt:KVM_DiskPool.InstanceID="notthere"'
returns
localhost:5988/root/virt:KVM_AllocationCapabilities.InstanceID="notthere"
Signed-off-by: Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
diff -r fd476f08b35e -r d4471d3ac476 src/Makefile.am
--- a/src/Makefile.am Thu Jan 10 14:20:52 2008 +0100
+++ b/src/Makefile.am Thu Jan 10 14:20:53 2008 +0100
@@ -89,17 +89,18 @@ libVirt_ElementConformsToProfile_la_LIBA
libVirt_EnabledLogicalElementCapabilities_la_SOURCES =
Virt_EnabledLogicalElementCapabilities.c
+libVirt_AllocationCapabilities_la_DEPENDENCIES = libVirt_DevicePool.la
+libVirt_AllocationCapabilities_la_SOURCES = Virt_AllocationCapabilities.c
+libVirt_AllocationCapabilities_la_LIBADD = -lVirt_DevicePool
+
libVirt_ElementCapabilities_la_DEPENDENCIES =
libVirt_VirtualSystemManagementCapabilities.la
libVirt_EnabledLogicalElementCapabilities.la libVirt_ComputerSystem.la
libVirt_HostSystem.la libVirt_VSMigrationCapabilities.la
libVirt_ElementCapabilities_la_SOURCES = Virt_ElementCapabilities.c
libVirt_ElementCapabilities_la_LIBADD = -lVirt_VirtualSystemManagementCapabilities \
-lVirt_EnabledLogicalElementCapabilities \
-lVirt_ComputerSystem \
-lVirt_HostSystem \
- -lVirt_VSMigrationCapabilities
-
-libVirt_AllocationCapabilities_la_DEPENDENCIES = libVirt_DevicePool.la
-libVirt_AllocationCapabilities_la_SOURCES = Virt_AllocationCapabilities.c
-libVirt_AllocationCapabilities_la_LIBADD = -lVirt_DevicePool
+ -lVirt_VSMigrationCapabilities \
+ -lVirt_AllocationCapabilities
libVirt_SettingsDefineCapabilities_la_DEPENDENCIES = libVirt_RASD.la
libVirt_DevicePool.la
libVirt_SettingsDefineCapabilities_la_SOURCES = Virt_SettingsDefineCapabilities.c
diff -r fd476f08b35e -r d4471d3ac476 src/Virt_ElementCapabilities.c
--- a/src/Virt_ElementCapabilities.c Thu Jan 10 14:20:52 2008 +0100
+++ b/src/Virt_ElementCapabilities.c Thu Jan 10 14:20:53 2008 +0100
@@ -37,6 +37,7 @@
#include "Virt_ComputerSystem.h"
#include "Virt_HostSystem.h"
#include "Virt_VSMigrationCapabilities.h"
+#include "Virt_AllocationCapabilities.h"
/* Associate an XXX_Capabilities to the proper XXX_ManagedElement.
*
@@ -46,6 +47,37 @@
*/
const static CMPIBroker *_BROKER;
+
+static CMPIStatus validate_host_caps_ref(const CMPIObjectPath *ref)
+{
+ CMPIStatus s = {CMPI_RC_OK, NULL};
+ CMPIInstance *inst;
+ const char *prop;
+ char* classname;
+
+ classname = class_base_name(CLASSNAME(ref));
+
+ if (STREQC(classname, "VirtualSystemManagementCapabilities")) {
+ s = get_vsm_cap(_BROKER, ref, &inst);
+ } else if (STREQC(classname, "VirtualSystemMigrationCapabilities")) {
+ s = get_migration_caps(ref, &inst, _BROKER);
+ }
+
+ if (s.rc != CMPI_RC_OK)
+ goto out;
+
+ prop = cu_compare_ref(ref, inst);
+ if (prop != NULL) {
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", prop);
+ }
+
+ out:
+ free(classname);
+
+ return s;
+}
static CMPIStatus sys_to_cap(const CMPIObjectPath *ref,
struct std_assoc_info *info,
@@ -53,22 +85,13 @@ static CMPIStatus sys_to_cap(const CMPIO
{
CMPIInstance *inst;
CMPIStatus s = {CMPI_RC_OK, NULL};
- const char *prop;
-
- if (!match_hypervisor_prefix(ref, info))
- return s;
-
- s = get_host_cs(_BROKER, ref, &inst);
- if (s.rc != CMPI_RC_OK)
- goto out;
-
- prop = cu_compare_ref(ref, inst);
- if (prop != NULL) {
- cu_statusf(_BROKER, &s,
- CMPI_RC_ERR_FAILED,
- "No such HostSystem (%s)", prop);
- goto out;
- }
+
+ if (!match_hypervisor_prefix(ref, info))
+ goto out;
+
+ s = validate_host_ref(_BROKER, ref);
+ if (s.rc != CMPI_RC_OK)
+ goto out;
s = get_vsm_cap(_BROKER, ref, &inst);
if (s.rc == CMPI_RC_OK)
@@ -90,7 +113,11 @@ static CMPIStatus cap_to_sys(const CMPIO
CMPIStatus s = {CMPI_RC_OK, NULL};
if (!match_hypervisor_prefix(ref, info))
- return s;
+ goto out;
+
+ s = validate_host_caps_ref(ref);
+ if (s.rc != CMPI_RC_OK)
+ goto out;
s = get_host_cs(_BROKER, ref, &inst);
if (s.rc != CMPI_RC_OK)
@@ -111,7 +138,11 @@ static CMPIStatus cs_to_cap(const CMPIOb
const char *sys_name = NULL;
if (!match_hypervisor_prefix(ref, info))
- return s;
+ goto out;
+
+ s = validate_domain_ref(_BROKER, ref);
+ if (s.rc != CMPI_RC_OK)
+ goto out;
if (cu_get_str_path(ref, "Name", &sys_name) != CMPI_RC_OK) {
cu_statusf(_BROKER, &s,
@@ -138,26 +169,30 @@ static CMPIStatus cap_to_cs(const CMPIOb
CMPIStatus s = {CMPI_RC_OK, NULL};
if (!match_hypervisor_prefix(ref, info))
- return s;
+ goto out;
if (cu_get_str_path(ref, "InstanceID", &inst_id) != CMPI_RC_OK) {
cu_statusf(_BROKER, &s,
CMPI_RC_ERR_FAILED,
"Could not get InstanceID");
- goto error1;
+ goto out;
}
conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s);
if (s.rc != CMPI_RC_OK)
- goto error1;
+ goto out;
inst = instance_from_name(_BROKER, conn, inst_id, ref);
if (inst)
inst_list_add(list, inst);
-
+ else
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", inst_id);
+
virConnectClose(conn);
- error1:
-
+
+ out:
return s;
}
@@ -173,14 +208,12 @@ static CMPIStatus pool_to_alloc(const CM
struct std_assoc_info *info,
struct inst_list *list)
{
- int ret;
+ CMPIStatus s = {CMPI_RC_OK, NULL};
+ CMPIInstance *inst = NULL;
const char *inst_id;
- uint16_t type;
- CMPIInstance *inst = NULL;
- CMPIStatus s = {CMPI_RC_OK};
-
- if (!match_hypervisor_prefix(ref, info))
- return s;
+
+ if (!match_hypervisor_prefix(ref, info))
+ goto out;
if (cu_get_str_path(ref, "InstanceID", &inst_id) != CMPI_RC_OK) {
cu_statusf(_BROKER, &s,
@@ -189,22 +222,16 @@ static CMPIStatus pool_to_alloc(const CM
goto out;
}
- inst = get_typed_instance(_BROKER,
- CLASSNAME(ref),
- "AllocationCapabilities",
- NAMESPACE(ref));
- CMSetProperty(inst, "InstanceID", inst_id, CMPI_chars);
-
- ret = cu_get_u16_path(ref, "ResourceType", &type);
- if (ret != 1) {
- cu_statusf(_BROKER, &s,
- CMPI_RC_ERR_FAILED,
- "Could not get ResourceType");
- goto out;
- }
- CMSetProperty(inst, "ResourceType", &type, CMPI_uint16);
-
- inst_list_add(list, inst);
+ s = alloc_cap_instances(_BROKER,
+ ref,
+ NULL,
+ false,
+ NULL,
+ inst_id,
+ &inst);
+
+ if (inst)
+ inst_list_add(list, inst);
out:
return s;
7:35 p.m.
New subject: [PATCH 3 of 3] EC: Validation of client given object path missing
Heidi Eckhart wrote:
# HG changeset patch
# User Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
# Date 1199971253 -3600
# Node ID d4471d3ac4766af68f6bf151813f0ec246b0e409
# Parent fd476f08b35e235ba7e50d326e5602018ebc3730
EC: Validation of client given object path missing
conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s);
if (s.rc != CMPI_RC_OK)
- goto error1;
+ goto out;
A question not directly related to this patch:
Shouldn't we also check to see if conn is NULL here? If
connect_by_classname() is unable to connect, an error value isn't
assigned to the CMPIStatus variable. Or possibly a better approach
would be to modify connect_by_classname() so that it sets an error.
--
Kaitlin Rupert
IBM Linux Technology Center
karupert(a)us.ibm.com
7:45 p.m.
New subject: [PATCH 3 of 3] EC: Validation of client given object path missing
KR> Or possibly a better approach would be to modify
KR> connect_by_classname() so that it sets an error.
I'm pretty sure that connect_by_classname() used to set an error in
the CMPIStatus if it was going to return NULL. I probably broke that
when I fixed up (yes, ironic) the connect_by_classname() stuff a
little while ago.
Good catch!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
12:17 p.m.
New subject: [PATCH 3 of 3] EC: Validation of client given object path missing
Dan Smith wrote:
KR> Or possibly a better approach would be to modify
KR> connect_by_classname() so that it sets an error.
I'm pretty sure that connect_by_classname() used to set an error in
the CMPIStatus if it was going to return NULL.
Yes, it did. But for consistency
with CIM this was removed. The reason
is that a request against KVM on a Xen system can not return with an
error, as this breaks the CIM model and operations. This happens if the
client does e.g. an ein on CIM_ComputerSystem. We know that this one
request gets split up into requests against Xen_ComputerSystem and
KVM_ComputerSystem by the CIMOM. The CIMOM would then only return the
error code of the KVM request and revoke the instances returned by the
Xen request. This behavior would be wrong.
I probably broke that
when I fixed up (yes, ironic) the connect_by_classname() stuff a
little while ago.
Good catch!
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
4:23 p.m.
New subject: [PATCH 3 of 3] EC: Validation of client given object path missing
Heidi Eckhart wrote:
Dan Smith wrote:
> KR> Or possibly a better approach would be to modify
> KR> connect_by_classname() so that it sets an error.
>
> I'm pretty sure that connect_by_classname() used to set an error in
> the CMPIStatus if it was going to return NULL.
Yes, it did. But for consistency with CIM this was removed. The reason
is that a request against KVM on a Xen system can not return with an
error, as this breaks the CIM model and operations. This happens if the
client does e.g. an ein on CIM_ComputerSystem. We know that this one
request gets split up into requests against Xen_ComputerSystem and
KVM_ComputerSystem by the CIMOM. The CIMOM would then only return the
error code of the KVM request and revoke the instances returned by the
Xen request. This behavior would be wrong.
Excellent point. Thanks for catching this. =)
--
Kaitlin Rupert
IBM Linux Technology Center
karupert(a)us.ibm.com
6091
days inactive
6097
days old
15 comments
3 participants
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert