[PATCH 0 of 3] Fix AC GetInstance() error
AC GetInstance() currently returns an error. This patchset fixes that error and also cleans up some left over functions.
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1197677154 28800 # Node ID 889611ecea877bb54fad494ece56b2e7fac2b98e # Parent 86999a1e8fac6f6cee5ca62a4005ca37759713ff AC GetInstance() fails with a "Could not get ResourceType" error. GetInstance() was calling return_alloc_cap(), which in turn called rasd_type_from_classname(). rasd_type_from_classname() is expecting the classname to be one of the supported RASD type classnames, but this doesn't match the AC classname. Instead, GetInstance() should only return an instance if the supplied InstanceID matches one of the existing instances. alloc_cap_instances() should be modified to take a InstanceID argument. Failing query: wbemcli gi 'http://localhost/root/virt:Xen_AllocationCapabilities.InstanceID="ProcessorPool/0"' Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 86999a1e8fac -r 889611ecea87 src/Virt_AllocationCapabilities.c --- a/src/Virt_AllocationCapabilities.c Wed Dec 12 17:22:06 2007 -0800 +++ b/src/Virt_AllocationCapabilities.c Fri Dec 14 16:05:54 2007 -0800 @@ -20,6 +20,7 @@ */ #include <stdlib.h> #include <unistd.h> +#include <string.h> #include <cmpidt.h> #include <cmpift.h> @@ -131,7 +132,8 @@ static CMPIStatus alloc_cap_instances(co const CMPIObjectPath *ref, const CMPIResult *results, bool names_only, - const char **properties) + const char **properties, + const char *id) { int i; virConnectPtr conn = NULL; @@ -139,6 +141,7 @@ static CMPIStatus alloc_cap_instances(co struct inst_list alloc_cap_list; struct inst_list device_pool_list; CMPIStatus s = {CMPI_RC_OK, NULL}; + const char *inst_id; CU_DEBUG("In alloc_cap_instances()"); @@ -165,6 +168,16 @@ static CMPIStatus alloc_cap_instances(co } for (i = 0; i < device_pool_list.cur; i++) { + if (cu_get_str_prop(device_pool_list.list[i], + "InstanceID", &inst_id) != CMPI_RC_OK) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Error fetching device pool InstanceID"); + goto out; + } + if (id && (!STREQ(inst_id, id))) + continue; + s = ac_from_pool(broker, ref, device_pool_list.list[i], &alloc_cap_inst); @@ -172,6 +185,9 @@ static CMPIStatus alloc_cap_instances(co goto out; inst_list_add(&alloc_cap_list, alloc_cap_inst); + + if (id && (STREQ(inst_id, id))) + break; } if (names_only) @@ -192,7 +208,22 @@ static CMPIStatus GetInstance(CMPIInstan const CMPIObjectPath *reference, const char **properties) { - return return_alloc_cap(reference, results, 0); + CMPIStatus s = {CMPI_RC_OK, NULL}; + const char* id; + + if (cu_get_str_path(reference, "InstanceID", &id) != CMPI_RC_OK) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "No InstanceID specified"); + return s; + } + + return alloc_cap_instances(_BROKER, + reference, + results, + false, + properties, + id); } static CMPIStatus EnumInstanceNames(CMPIInstanceMI *self, @@ -200,7 +231,7 @@ static CMPIStatus EnumInstanceNames(CMPI const CMPIResult *results, const CMPIObjectPath *reference) { - return alloc_cap_instances(_BROKER, reference, results, true, NULL); + return alloc_cap_instances(_BROKER, reference, results, true, NULL, NULL); } static CMPIStatus EnumInstances(CMPIInstanceMI *self, @@ -209,7 +240,7 @@ static CMPIStatus EnumInstances(CMPIInst const CMPIObjectPath *reference, const char **properties) { - return alloc_cap_instances(_BROKER, reference, results, false, properties); + return alloc_cap_instances(_BROKER, reference, results, false, properties, NULL); } DEFAULT_CI();
Kaitlin Rupert wrote:
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1197677154 28800 # Node ID 889611ecea877bb54fad494ece56b2e7fac2b98e # Parent 86999a1e8fac6f6cee5ca62a4005ca37759713ff AC GetInstance() fails with a "Could not get ResourceType" error.
GetInstance() was calling return_alloc_cap(), which in turn called rasd_type_from_classname(). rasd_type_from_classname() is expecting the classname to be one of the supported RASD type classnames, but this doesn't match the AC classname.
Instead, GetInstance() should only return an instance if the supplied InstanceID matches one of the existing instances. alloc_cap_instances() should be modified to take a InstanceID argument.
Failing query: wbemcli gi 'http://localhost/root/virt:Xen_AllocationCapabilities.InstanceID="ProcessorPool/0"'
Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com>
const char **properties) { - return return_alloc_cap(reference, results, 0); + CMPIStatus s = {CMPI_RC_OK, NULL}; + const char* id; + + if (cu_get_str_path(reference, "InstanceID", &id) != CMPI_RC_OK) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "No InstanceID specified"); + return s; + }
Just a general style thing here. Don't know if we've made it official or not but most of the time we would use a "goto out;" type thing there. Nothing big but if you need to resend to fix that segfault Heidi found this would probably be worth changing for the sake of consistency. -- -Jay
Jay Gagnon wrote:
+ + if (cu_get_str_path(reference, "InstanceID", &id) != CMPI_RC_OK) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "No InstanceID specified"); + return s; + }
Just a general style thing here. Don't know if we've made it official or not but most of the time we would use a "goto out;" type thing there. Nothing big but if you need to resend to fix that segfault Heidi found this would probably be worth changing for the sake of consistency.
Right, good point. I didn't use "goto out" here because if we don't encounter an error, we do the following: + return alloc_cap_instances(_BROKER, + reference, + results, + false, + properties, + id); So "goto out" would just skip past this return in order to return the status. But since I need to fix this patch anyway, I can make this change as well. =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
KR> Right, good point. I didn't use "goto out" here because if we don't KR> encounter an error, we do the following: Actually, I think that's a good reason to just return from where you do. I'm sure anything trying to allow the "goto out" will be ugly :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
KR> Right, good point. I didn't use "goto out" here because if we don't KR> encounter an error, we do the following:
Actually, I think that's a good reason to just return from where you do. I'm sure anything trying to allow the "goto out" will be ugly :)
Well, I'd definitely rather have "slightly different than normal" over "really ugly" here. I didn't look too hard at the surrounding code, so I didn't realize that it will get messy trying to use goto out. No worries, leave it as is. -- -Jay
Jay Gagnon wrote:
Well, I'd definitely rather have "slightly different than normal" over "really ugly" here. I didn't look too hard at the surrounding code, so I didn't realize that it will get messy trying to use goto out. No worries, leave it as is.
Thanks - that works for me =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1197677158 28800 # Node ID 552b27c0e224e278d9b8c12e39b45808667345c1 # Parent 889611ecea877bb54fad494ece56b2e7fac2b98e Clean up unneeded functions in AC. Now that GetInstance is calling alloc_cap_instances(), get_alloc_cap() and return_alloc_cap() are no longer needed. This removes the need for Virt_AllocationCapabilities.h Also clean up lengthy function calls. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 889611ecea87 -r 552b27c0e224 src/Virt_AllocationCapabilities.c --- a/src/Virt_AllocationCapabilities.c Fri Dec 14 16:05:54 2007 -0800 +++ b/src/Virt_AllocationCapabilities.c Fri Dec 14 16:05:58 2007 -0800 @@ -31,66 +31,9 @@ #include "misc_util.h" -#include "Virt_AllocationCapabilities.h" -#include "Virt_RASD.h" #include "Virt_DevicePool.h" const static CMPIBroker *_BROKER; - -CMPIStatus get_alloc_cap(const CMPIBroker *broker, - const CMPIObjectPath *ref, - CMPIInstance **inst) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - char *inst_id; - uint16_t type; - int ret; - - *inst = get_typed_instance(broker, - CLASSNAME(ref), - "AllocationCapabilities", - NAMESPACE(ref)); - - if (rasd_type_from_classname(CLASSNAME(ref), &type) != CMPI_RC_OK) { - cu_statusf(broker, &s, - CMPI_RC_ERR_FAILED, - "Could not get ResourceType"); - goto out; - } - - ret = asprintf(&inst_id, "%hi/%s", type, "0"); - if (ret == -1) { - cu_statusf(broker, &s, - CMPI_RC_ERR_FAILED, - "Could not get InstanceID"); - goto out; - } - - CMSetProperty(*inst, "InstanceID", inst_id, CMPI_chars); - CMSetProperty(*inst, "ResourceType", &type, CMPI_uint16); - - out: - return s; -} - -static CMPIStatus return_alloc_cap(const CMPIObjectPath *ref, - const CMPIResult *results, - int names_only) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - CMPIInstance *inst = NULL; - - s = get_alloc_cap(_BROKER, ref, &inst); - if (s.rc != CMPI_RC_OK) - goto out; - - if (names_only) - cu_return_instance_name(results, inst); - else - CMReturnInstance(results, inst); - out: - return s; -} static CMPIStatus ac_from_pool(const CMPIBroker *broker, const CMPIObjectPath *ref, @@ -231,7 +174,12 @@ static CMPIStatus EnumInstanceNames(CMPI const CMPIResult *results, const CMPIObjectPath *reference) { - return alloc_cap_instances(_BROKER, reference, results, true, NULL, NULL); + return alloc_cap_instances(_BROKER, + reference, + results, + true, + NULL, + NULL); } static CMPIStatus EnumInstances(CMPIInstanceMI *self, @@ -240,7 +188,12 @@ static CMPIStatus EnumInstances(CMPIInst const CMPIObjectPath *reference, const char **properties) { - return alloc_cap_instances(_BROKER, reference, results, false, properties, NULL); + return alloc_cap_instances(_BROKER, + reference, + results, + false, + properties, + NULL); } DEFAULT_CI(); diff -r 889611ecea87 -r 552b27c0e224 src/Virt_AllocationCapabilities.h --- a/src/Virt_AllocationCapabilities.h Fri Dec 14 16:05:54 2007 -0800 +++ /dev/null Thu Jan 01 00:00:00 1970 +0000 @@ -1,32 +0,0 @@ -/* - * Copyright IBM Corp. 2007 - * - * Authors: - * Jay Gagnon <grendel@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 - */ -CMPIStatus get_alloc_cap(const CMPIBroker *broker, - const CMPIObjectPath *ref, - CMPIInstance **inst); -/* - * Local Variables: - * mode: C - * c-set-style: "K&R" - * tab-width: 8 - * c-basic-offset: 8 - * indent-tabs-mode: nil - * End: - */
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1197677598 28800 # Node ID a741d41893ec632315fe905aa07ce4568c318cd3 # Parent 552b27c0e224e278d9b8c12e39b45808667345c1 AC no longer needs libVirt_RASD. This cleans up the AC build dependencies. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 552b27c0e224 -r a741d41893ec src/Makefile.am --- a/src/Makefile.am Fri Dec 14 16:05:58 2007 -0800 +++ b/src/Makefile.am Fri Dec 14 16:13:18 2007 -0800 @@ -2,7 +2,6 @@ SUBDIRS = tests SUBDIRS = tests noinst_HEADERS = profiles.h svpc_types.h \ - Virt_AllocationCapabilities.h \ Virt_ComputerSystem.h \ Virt_ComputerSystemIndication.h \ Virt_Device.h \ @@ -98,9 +97,9 @@ libVirt_ElementCapabilities_la_LIBADD = -lVirt_HostSystem \ -lVirt_VSMigrationCapabilities -libVirt_AllocationCapabilities_la_DEPENDENCIES = libVirt_RASD.la libVirt_DevicePool.la +libVirt_AllocationCapabilities_la_DEPENDENCIES = libVirt_DevicePool.la libVirt_AllocationCapabilities_la_SOURCES = Virt_AllocationCapabilities.c -libVirt_AllocationCapabilities_la_LIBADD = -lVirt_RASD -lVirt_DevicePool +libVirt_AllocationCapabilities_la_LIBADD = -lVirt_DevicePool libVirt_SettingsDefineCapabilities_la_DEPENDENCIES = libVirt_RASD.la libVirt_DevicePool.la libVirt_SettingsDefineCapabilities_la_SOURCES = Virt_SettingsDefineCapabilities.c @@ -146,4 +145,5 @@ libVirt_ElementSettingData_la_LIBADD = - libVirt_VSMigrationCapabilities_la_SOURCES = Virt_VSMigrationCapabilities.c -libVirt_VSMigrationService_la_SOURCES = Virt_VSMigrationService.c \ No newline at end of file +libVirt_VSMigrationService_la_SOURCES = Virt_VSMigrationService.c +
Kaitlin Rupert wrote:
libVirt_VSMigrationCapabilities_la_SOURCES = Virt_VSMigrationCapabilities.c
-libVirt_VSMigrationService_la_SOURCES = Virt_VSMigrationService.c \ No newline at end of file +libVirt_VSMigrationService_la_SOURCES = Virt_VSMigrationService.c +
I'm unsure why this showed up in the diff - I didn't make any changes to this line. My apologies for the fuzz.. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Kaitlin Rupert wrote:
Kaitlin Rupert wrote:
libVirt_VSMigrationCapabilities_la_SOURCES = Virt_VSMigrationCapabilities.c
-libVirt_VSMigrationService_la_SOURCES = Virt_VSMigrationService.c \ No newline at end of file +libVirt_VSMigrationService_la_SOURCES = Virt_VSMigrationService.c +
I'm unsure why this showed up in the diff - I didn't make any changes to this line. My apologies for the fuzz..
That kind of thing happens to me sometimes to, and it's very annoying cuz it's an easy mistake to make. It's entirely forgiveable though. It's officially on my (oh so secret) list of exceptions to the guidelines, because it's such a stupid thing to have to look out for. I believe it is related to how emacs does certain operations, like "delete the rest of this line" and that kind of thing. Regardless, any time a file with no newline at the end has one added, all nitpicking charges will be dropped. Removing the ending newline will not be overlooked, however, because things will be cleaner if all files always have newline at the end. Hopefully that doesn't sound as arbitrary as it feels. -- -Jay
JG> Regardless, any time a file with no newline at the end has one JG> added, all nitpicking charges will be dropped. Removing the JG> ending newline will not be overlooked, however, because things JG> will be cleaner if all files always have newline at the end. Agreed. Actually, it is probably best if we try to notice when new patches don't have a newline to reduce the potential for this. I don't know about less-sophisticated editors, but in emacs, you can add this to your .emacs to ensure all files have a final newline: (setq require-final-newline t) I thought the default was to either force or ask, but maybe some people don't have this set in their environment. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> Regardless, any time a file with no newline at the end has one JG> added, all nitpicking charges will be dropped. Removing the JG> ending newline will not be overlooked, however, because things JG> will be cleaner if all files always have newline at the end.
Thanks for the explanation. =)
Agreed. Actually, it is probably best if we try to notice when new patches don't have a newline to reduce the potential for this.
I don't know about less-sophisticated editors, but in emacs, you can add this to your .emacs to ensure all files have a final newline:
(setq require-final-newline t)
I thought the default was to either force or ask, but maybe some people don't have this set in their environment.
I'm using vim, and I'm not sure what the default action for this is. I'll look into it. -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Kaitlin Rupert wrote:
AC GetInstance() currently returns an error. This patchset fixes that error and also cleans up some left over functions.
This fix works perfectly for me. Great :). The only problem I encountered was with a non existing instance, e.g. a typo in the InstanceID as below. Therefore the provider segfaults. Please can you have a look into this ? Thanks. wbemcli gi 'http://localhost/root/virt:Xen_AllocationCapabilities.InstanceID="ProcessorPol/0"' -- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Heidi Eckhart wrote:
The only problem I encountered was with a non existing instance, e.g. a typo in the InstanceID as below. Therefore the provider segfaults. Please can you have a look into this ? Thanks.
wbemcli gi 'http://localhost/root/virt:Xen_AllocationCapabilities.InstanceID="ProcessorPol/0"'
Thanks Heidi for testing this. Are you testing on sfcb? I tested this same scenario on Pegasus (before submitting), and I got the following error: * * wbemcli: Cim: (6) CIM_ERR_NOT_FOUND: The requested object could not be found * I just tried applying these patches on a clean tree and re-testing - I get the same error. I'm testing with tog-pegasus-2.6.1-2.fc8. Has anyone else encountered a segfault or other unexpected error condition? -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
Kaitlin Rupert wrote:
Heidi Eckhart wrote:
The only problem I encountered was with a non existing instance, e.g. a typo in the InstanceID as below. Therefore the provider segfaults. Please can you have a look into this ? Thanks.
wbemcli gi 'http://localhost/root/virt:Xen_AllocationCapabilities.InstanceID="ProcessorPol/0"'
Thanks Heidi for testing this. Are you testing on sfcb? Yes, testing on sfcb. I tested this same scenario on Pegasus (before submitting), and I got the following error:
* * wbemcli: Cim: (6) CIM_ERR_NOT_FOUND: The requested object could not be found *
Well, yet another difference between Pegasus and sfcb. Pegasus seems to take the given object path, do an enumerateInstanceNames and iterate through the returned instances trying to find the requested one. This error_not_found is definitely created by Pegasus itself, while the getInstance() of our provider does not get called in this case. On the other hand sfcb directs this call to the provider (that's why he can call himself "small footprint" ;) ).
I just tried applying these patches on a clean tree and re-testing - I get the same error. I'm testing with tog-pegasus-2.6.1-2.fc8.
Has anyone else encountered a segfault or other unexpected error condition?
I will have a look into it and try to find out where it segfault. -- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Heidi Eckhart wrote:
I will have a look into it and try to find out where it segfault.
After some testing and debugging, I figured out that sfcb seems to have a problem with a return result "no instance and status OK" for getInstance(). This should not cause a segfault in sfcb, but the behavior in general is correct, as this result is not valid for getInstance. If the requested instances was not found, then the status has to be set to something else than OK. I will send this issue to the sblim-devel mailing list. This patch fixes the getInstance() segfault with sfcb. diff -r 84b0269e9994 src/Virt_AllocationCapabilities.c --- a/src/Virt_AllocationCapabilities.c Tue Dec 18 12:20:13 2007 +0100 +++ b/src/Virt_AllocationCapabilities.c Tue Dec 18 12:21:53 2007 +0100 @@ -118,8 +118,10 @@ static CMPIStatus alloc_cap_instances(co "Error fetching device pool InstanceID"); goto out; } - if (id && (!STREQ(inst_id, id))) + if (id && (!STREQ(inst_id, id))) { + inst_id = NULL; continue; + } s = ac_from_pool(broker, ref, device_pool_list.list[i], @@ -131,6 +133,13 @@ static CMPIStatus alloc_cap_instances(co if (id && (STREQ(inst_id, id))) break; + } + + if (id && !inst_id) { + cu_statusf(broker, &s, + CMPI_RC_ERR_NOT_FOUND, + "Requested Object could not be found."); + goto out; } if (names_only) -- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Heidi Eckhart wrote:
Heidi Eckhart wrote:
I will have a look into it and try to find out where it segfault.
After some testing and debugging, I figured out that sfcb seems to have a problem with a return result "no instance and status OK" for getInstance(). This should not cause a segfault in sfcb, but the behavior in general is correct, as this result is not valid for getInstance. If the requested instances was not found, then the status has to be set to something else than OK. I will send this issue to the sblim-devel mailing list.
Thanks for looking into this. =) Sounds like an interesting bug - good catch!
This patch fixes the getInstance() segfault with sfcb.
diff -r 84b0269e9994 src/Virt_AllocationCapabilities.c --- a/src/Virt_AllocationCapabilities.c Tue Dec 18 12:20:13 2007 +0100 +++ b/src/Virt_AllocationCapabilities.c Tue Dec 18 12:21:53 2007 +0100 @@ -118,8 +118,10 @@ static CMPIStatus alloc_cap_instances(co "Error fetching device pool InstanceID"); goto out; } - if (id && (!STREQ(inst_id, id))) + if (id && (!STREQ(inst_id, id))) { + inst_id = NULL; continue; + } s = ac_from_pool(broker, ref, device_pool_list.list[i], @@ -131,6 +133,13 @@ static CMPIStatus alloc_cap_instances(co
if (id && (STREQ(inst_id, id))) break; + } + + if (id && !inst_id) { + cu_statusf(broker, &s, + CMPI_RC_ERR_NOT_FOUND, + "Requested Object could not be found."); + goto out; }
if (names_only)
I can send out a re-worked patch. Thanks for coding this up. =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
participants (4)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon -
Kaitlin Rupert