6:27 p.m.
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(a)linux.vnet.ibm.com