[PATCH 0 of 3] Check client given object pathes in ElementCapabilities provider
To enable the check for the client's given object path in ElementCapabilities, changes to the ComputerSystem and AllocationCapabilities provider have been necessary.
# HG changeset patch # User Heidi Eckhart <heidieck@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@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
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@us.ibm.com
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
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@us.ibm.com
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
# HG changeset patch # User Heidi Eckhart <heidieck@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@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@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: + */
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@us.ibm.com
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
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@us.ibm.com
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
# HG changeset patch # User Heidi Eckhart <heidieck@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@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;
Heidi Eckhart wrote:
# HG changeset patch # User Heidi Eckhart <heidieck@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@us.ibm.com
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@us.ibm.com
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
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@us.ibm.com
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert