1:04 p.m.
# HG changeset patch
# User Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
# Date 1199797461 -3600
# Node ID 555126035e912e9202ebcc93483ee936be5ae998
# Parent 1bbea87ad37dcb95321f43aa5e17e6fa7ec1836d
HS: returns results for wrong object path
The client given reference was not checked for existance in both
directions (from HostSystem to Service and reverse) and the call
returned wrong results. Now NOT_FOUND is returned in case of a
wrong object path.
Examples:
wbemcli ain -ac KVM_HostedService
'http://localhost/root/virt:KVM_ResourcePoolConfigurationService.CreationClassName="wrong",wrong="wrong"'
wbemcli ain -ac KVM_HostedService
'http://localhost/root/virt:KVM_HostSystem.CreationClassName="KVM_HostSystem",Name="notthere"'
Signed-off-by: Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
diff -r 1bbea87ad37d -r 555126035e91 src/Virt_HostedService.c
--- a/src/Virt_HostedService.c Mon Jan 07 11:42:40 2008 -0800
+++ b/src/Virt_HostedService.c Tue Jan 08 14:04:21 2008 +0100
@@ -37,6 +37,57 @@
const static CMPIBroker *_BROKER;
+static CMPIStatus check_host_ref(const CMPIObjectPath *ref)
+{
+ CMPIStatus s = {CMPI_RC_OK, NULL};
+ CMPIInstance *inst;
+ const char *prop;
+
+ s = get_host_cs(_BROKER, ref, &inst);
+
+ prop = cu_compare_ref(ref, inst);
+ if (prop != NULL) {
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", prop);
+ }
+
+ return s;
+}
+
+static CMPIStatus check_service_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, "VirtualSystemManagementService")) {
+ s = get_vsms(ref, &inst, _BROKER);
+ }
+ else if (STREQC(classname, "ResourcePoolConfigurationService")) {
+ s = rpcs_instance(ref, &inst, _BROKER);
+ }
+ else if (STREQC(classname, "VirtualSystemMigrationService")) {
+ s = get_migration_service(ref, &inst, _BROKER);
+ }
+ if (s.rc != CMPI_RC_OK)
+ return s;
+
+ prop = cu_compare_ref(ref, inst);
+ if (prop != NULL) {
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", prop);
+ }
+
+ free(classname);
+
+ return s;
+}
+
static CMPIStatus service_to_host(const CMPIObjectPath *ref,
struct std_assoc_info *info,
struct inst_list *list)
@@ -47,6 +98,10 @@ static CMPIStatus service_to_host(const
if (!match_hypervisor_prefix(ref, info))
return s;
+ s = check_service_ref(ref);
+ if (s.rc != CMPI_RC_OK)
+ return s;
+
s = get_host_cs(_BROKER, ref, &instance);
if (s.rc == CMPI_RC_OK)
inst_list_add(list, instance);
@@ -62,6 +117,10 @@ static CMPIStatus host_to_service(const
CMPIInstance *inst;
if (!match_hypervisor_prefix(ref, info))
+ return s;
+
+ s = check_host_ref(ref);
+ if (s.rc != CMPI_RC_OK)
return s;
s = rpcs_instance(ref, &inst, _BROKER);
3:12 p.m.
Heidi Eckhart wrote:
# HG changeset patch
# User Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
# Date 1199797461 -3600
# Node ID 555126035e912e9202ebcc93483ee936be5ae998
# Parent 1bbea87ad37dcb95321f43aa5e17e6fa7ec1836d
HS: returns results for wrong object path
The client given reference was not checked for existance in both
directions (from HostSystem to Service and reverse) and the call
returned wrong results. Now NOT_FOUND is returned in case of a
wrong object path.
Examples:
wbemcli ain -ac KVM_HostedService
'http://localhost/root/virt:KVM_ResourcePoolConfigurationService.CreationClassName="wrong",wrong="wrong"'
wbemcli ain -ac KVM_HostedService
'http://localhost/root/virt:KVM_HostSystem.CreationClassName="KVM_HostSystem",Name="notthere"'
Signed-off-by: Heidi Eckhart <heidieck(a)linux.vnet.ibm.com>
diff -r 1bbea87ad37d -r 555126035e91 src/Virt_HostedService.c
--- a/src/Virt_HostedService.c Mon Jan 07 11:42:40 2008 -0800
+++ b/src/Virt_HostedService.c Tue Jan 08 14:04:21 2008 +0100
@@ -37,6 +37,57 @@
+
+static CMPIStatus check_service_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, "VirtualSystemManagementService")) {
+ s = get_vsms(ref, &inst, _BROKER);
+ }
+ else if (STREQC(classname, "ResourcePoolConfigurationService")) {
+ s = rpcs_instance(ref, &inst, _BROKER);
+ }
+ else if (STREQC(classname, "VirtualSystemMigrationService")) {
+ s = get_migration_service(ref, &inst, _BROKER);
+ }
+ if (s.rc != CMPI_RC_OK)
+ return s;
Sorry for being so nitpicky, but for the sake of consistency I think
this is a place where we use a "goto out;" type of statement as opposed
to having multiple returns.
+
+ prop = cu_compare_ref(ref, inst);
+ if (prop != NULL) {
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_NOT_FOUND,
+ "No such instance (%s)", prop);
+ }
+
Putting out right here should work fine. Actually, this is more than a
nitpick I think, because if the above return happens we don't hit this
free() and leak classname.
+ free(classname);
+
+ return s;
+}
--
-Jay
10:14 a.m.
Jay Gagnon wrote:
Heidi Eckhart wrote:
> + if (s.rc != CMPI_RC_OK)
> + return s;
>
>
Sorry for being so nitpicky, but for the sake of consistency I think
this is a place where we use a "goto out;" type of statement as opposed
to having multiple returns.
I wouldn't call it nitpicky. Its a very good finding :) ! Thanks.
> +
> + prop = cu_compare_ref(ref, inst);
> + if (prop != NULL) {
> + cu_statusf(_BROKER, &s,
> + CMPI_RC_ERR_NOT_FOUND,
> + "No such instance (%s)", prop);
> + }
> +
>
>
Putting out right here should work fine.
Agree and done so in the updated patch.
Actually, this is more than a
nitpick I think, because if the above return happens we don't hit this
free() and leak classname.
Mhh ... the above code does only set the status, but does not return
right afterwards. So we wouldn't leak classname. Or did I miss something ?
> + free(classname);
> +
> + return s;
> +}
>
>
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
4:22 p.m.
Heidi Eckhart wrote:
Jay Gagnon wrote:
> Heidi Eckhart wrote:
>
>> + if (s.rc != CMPI_RC_OK)
>> + return s;
>>
> Sorry for being so nitpicky, but for the sake of consistency I think
> this is a place where we use a "goto out;" type of statement as opposed
> to having multiple returns.
>
I wouldn't call it nitpicky. Its a very good finding :) ! Thanks.
>> + + prop = cu_compare_ref(ref, inst);
>> + if (prop != NULL) {
>> + cu_statusf(_BROKER, &s,
>> + CMPI_RC_ERR_NOT_FOUND,
>> + "No such instance (%s)", prop);
>> + }
>> +
> Putting out right here should work fine.
Agree and done so in the updated patch.
> Actually, this is more than a
> nitpick I think, because if the above return happens we don't hit this
> free() and leak classname.
>
Mhh ... the above code does only set the status, but does not return
right afterwards. So we wouldn't leak classname. Or did I miss
something ?
Sorry, my "above" was a bit vague. I meant that if the
"return s;" at
the very top of this email happens, we leave the function before we get
to "free(classname);" and so classname will be leaked. But no matter,
the point will become moot when you switch the return to a goto;
classname definitely won't leak that way.
--
-Jay
4:04 p.m.
HE> + if (STREQC(classname, "VirtualSystemManagementService")) {
HE> + s = get_vsms(ref, &inst, _BROKER);
HE> + }
HE> + else if (STREQC(classname, "ResourcePoolConfigurationService"))
{
HE> + s = rpcs_instance(ref, &inst, _BROKER);
HE> + }
HE> + else if (STREQC(classname, "VirtualSystemMigrationService")) {
HE> + s = get_migration_service(ref, &inst, _BROKER);
HE> + }
HE> + if (s.rc != CMPI_RC_OK)
HE> + return s;
This doesn't fit the style of the rest of the code. The "else if"
statements should be on the same line as the closing brace, and there
should be a blank line before the second and unrelated if.
Otherwise, a good fix. Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
10:14 a.m.
Dan Smith wrote:
HE> + if (STREQC(classname,
"VirtualSystemManagementService")) {
HE> + s = get_vsms(ref, &inst, _BROKER);
HE> + }
HE> + else if (STREQC(classname, "ResourcePoolConfigurationService"))
{
HE> + s = rpcs_instance(ref, &inst, _BROKER);
HE> + }
HE> + else if (STREQC(classname, "VirtualSystemMigrationService")) {
HE> + s = get_migration_service(ref, &inst, _BROKER);
HE> + }
HE> + if (s.rc != CMPI_RC_OK)
HE> + return s;
This doesn't fit the style of the rest of the code. The "else if"
statements should be on the same line as the closing brace, and there
should be a blank line before the second and unrelated if.
Fixed in the updated version.
--
Regards
Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor
6098
days inactive
6099
days old
5 comments
3 participants
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon