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

[Dan Smith]

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?

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> +{
KR> +        int i;
KR> +        int ret;
KR> +
KR> +        if (CMIsNullValue(data) || CMIsNullObject(data.value.array)) {
KR> +                CMSetStatus(s, CMPI_RC_ERR_INVALID_PARAMETER);
KR> +                CU_DEBUG("Method parameter is NULL");
KR> +                return 0;
KR> +        }
KR> +
KR> +        for (i = 0; i < count; i++) {
KR> +                CMPIData item;
KR> +                CMPIInstance *inst;
KR> +
KR> +                item = CMGetArrayElementAt(data.value.array, i, NULL);
KR> +
KR> +                ret = parse_eo_inst_arg(&inst,
KR> +                                        broker,
KR> +                                        item.value.string,
KR> +                                        ns,
KR> +                                        s);
KR> +
KR> +                if (ret != 1) {
KR> +                        CU_DEBUG("Parsing argument type %d failed", item.type);
KR> +                        return 0;
KR> +                }
KR> +
KR> +                CMSetArrayElementAt(*array, i,
KR> +                                    (CMPIValue *)&inst,
KR> +                                    CMPI_instance);
KR> +        }
KR> +
KR> +        return 1;
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.

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?

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.

-- 
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/20080128/9bfbbd69/attachment.sig>