[Libvirt-cim] [PATCH 1 of 2] [CU] Add embedded object parse functionality to std_invokemethod

[Kaitlin Rupert]

Dan Smith wrote:
> KR> +static int check_for_eo(CMPIType type, CMPIType exp_type, CMPIType *ret_type)
> KR> +{
> KR> +     if ((exp_type == CMPI_instance) && (type == CMPI_string)) {
> KR> +             *ret_type = CMPI_instance;
> KR> +             return 1;
> KR> +     }
> KR> +
> KR> +     if ((exp_type == CMPI_instanceA) && (type == CMPI_stringA)) {
> KR> +             *ret_type = CMPI_instanceA;
> KR> +             return 1;
> KR> +     }
> KR> +
> KR> +     *ret_type = -1;
> KR> +     return 0;
> KR> +}
> 
> Instead of taking three CMPIType arguments and returning an int, would
> it be cleaner to return a CMPIType, and use CMPI_null to signal a
> failure?

Yes - this is an excellent suggestion thanks. I looked for something 
like CMPI_null, but must have missed it in cmpidt.h.

> 
> KR> +static int parse_eo_array(CMPIArray **array,
> KR> +                          const CMPIBroker *broker,
> KR> +                          CMPIData data,
> KR> +                          const char *ns,
> KR> +                          int count,
> KR> +                          CMPIStatus *s)
> KR> +{
> 
> It seems to me that it would be less confusing to make the above
> function's signature look like this:
> 
>   static inst parse_eo_array(CMPIArray *strings_in,
>                              CMPIArray **instances_out,
>                              const CMPIBroker *broker,
>                              const char *ns,
>                              CMPIStatus *s);
> 
> If you do this, then you don't need count, and you're not passing
> around an ambiguously-typed CMPIData.

Good call.

> 
> KR> +static int parse_eo_param(CMPIArgs **args,
> KR> +                          const CMPIBroker *broker,
> KR> +                          CMPIData data,
> KR> +                          CMPIType type,
> KR> +                          const char *arg_name,
> KR> +                          const char *ns,
> KR> +                          CMPIStatus *s)
> KR> +{
> 
> Hmm, do we need args as a double-pointer?

Yes, since modify args.  This is the new arg element that will 
eventually be returned.  However, based on the suggestion above, I think 
things can be reworked so that we won't need the double-pointer.

> 
> KR> +        CMPIArray *new_array;
> KR> +        CMPIInstance *inst;
> KR> +        int ret;
> KR> +        int count;
> KR> +
> KR> +        if (type == CMPI_instance) {
> KR> +                ret = parse_eo_inst_arg(&inst,
> KR> +                                        broker,
> KR> +                                        data.value.string,
> KR> +                                        ns,
> KR> +                                        s);
> KR> +
> KR> +                if (ret != 1)
> KR> +                        return 0;
> KR> +
> KR> +                *s = CMAddArg(*args,
> KR> +                              arg_name,
> KR> +                              &(inst),
> KR> +                              CMPI_instance);
> KR> +        } else if (type == CMPI_instanceA) {
> KR> +                count = CMGetArrayCount(data.value.array, NULL);
> KR> +
> KR> +                new_array = CMNewArray(broker, count, CMPI_instance, s);
> KR> +
> KR> +                ret = parse_eo_array(&new_array,
> KR> +                                     broker,
> KR> +                                     data,
> KR> +                                     ns,
> KR> +                                     count,
> KR> +                                     s);
> KR> +                if (ret != 1)
> KR> +                        return 0;
> KR> +
> KR> +                *s = CMAddArg(*args,
> KR> +                              arg_name,
> KR> +                              &(new_array),
> KR> +                              CMPI_instanceA);
> 
> I think all of the above could become something simpler, like this:
> 
>   if (type == CMPI_instance)
>     ret = parse_eo_inst_arg(&newdata.value.inst,
>                             broker,
>                             data.value.string,
>                             ns,
>                             s);
>   else if (type == CMPI_instanceA)
>     ret = parse_eo_array(data.value.array,
>                          &newdata.value.array,
>                          broker,
>                          ns,
>                          s);
>   else
>     /* ERROR */
>   
>   if (ret != 1)
>     return 0;
>   
>   *s = CMAddArg(*args, args, arg_name, &newdata.value, type);
>   
>   return 1;
> 
> (with some of the details removed)... Right?  Might be nice to arrange
> the parameters of the two similar calls so they match up in order
> too.
> 
> Anyway, nothing major, but I think that some or all of the above could
> help clean it up a bit.

I was concerned about setting the CMPIValueState state info correctly, 
but if we add just the value of the CMPIData element to the array, then 
we don't need to be concerned with the state.  Yep, I think it's a good 
suggestion.

Thanks!

-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com