[Libvirt-cim] [PATCH 2 of 3] Make alloc_cap_instances() available for external use by EC

[Dan Smith]

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 at us.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvirt-cim/attachments/20080110/61c4e031/attachment.sig>