[PATCH 0 of 2] [RFC] [CU] Add EO parsing functionality to libcmpiutil.
During the validate method arguments call, ensure embedded objects have been parsed if the CIMOM hasn't done so already.
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1201551292 28800 # Node ID a6352980c902f4a0e3d60e97b4c2c9bc6c594674 # Parent 2948ecbed2674aee61a994b06468c2ce9c763bb3 [CU] Add embedded object parse functionality to std_invokemethod. This adds 3 functions: -check_for_eo(): Determine whether the argument is an embedded object that needs to be parsed. -parse_eo_inst_arg(): Calls cu_parse_embedded_instance() to parse the arg. -parse_eo_array(): Calls parse_eo_inst_arg() for each item in the arry. -parse_eo_param(): If argument is an array, walks through the array calling parse_eo_inst_arg(), otherwise call parse_eo_inst_arg() directly. parse_eo_inst_arg() and parse_eo_param() are based on the EO parsing pieces in Virt_VirtualSystemManagementService. The style of these changes matches the style in the rest of the file - return 0 for failure, return 1 for success. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r 2948ecbed267 -r a6352980c902 std_invokemethod.c --- a/std_invokemethod.c Tue Jan 15 12:54:04 2008 +0100 +++ b/std_invokemethod.c Mon Jan 28 12:14:52 2008 -0800 @@ -29,6 +29,154 @@ #include "std_invokemethod.h" #define STREQ(a,b) (strcmp(a, b) == 0) + +static int check_for_eo(CMPIType type, CMPIType exp_type, CMPIType *ret_type) +{ + if ((exp_type == CMPI_instance) && (type == CMPI_string)) { + *ret_type = CMPI_instance; + return 1; + } + + if ((exp_type == CMPI_instanceA) && (type == CMPI_stringA)) { + *ret_type = CMPI_instanceA; + return 1; + } + + *ret_type = -1; + return 0; +} + +static int parse_eo_inst_arg(CMPIInstance **inst, + const CMPIBroker *broker, + CMPIString *val, + const char *ns, + CMPIStatus *s) +{ + int ret; + const char *str; + + str = CMGetCharPtr(val); + + if (str == NULL) { + CMSetStatus(s, CMPI_RC_ERR_INVALID_PARAMETER); + CU_DEBUG("Method parameter value is NULL"); + return 0; + } + + ret = cu_parse_embedded_instance(str, + broker, + ns, + inst); + + /* cu_parse_embedded_instance() returns 0 on success */ + if ((ret != 0) || CMIsNullObject(inst)) { + CMSetStatus(s, CMPI_RC_ERR_FAILED); + CU_DEBUG("Unable to parse embedded object"); + return 0; + } + + return 1; +} + +static int parse_eo_array(CMPIArray **array, + const CMPIBroker *broker, + CMPIData data, + const char *ns, + int count, + CMPIStatus *s) +{ + int i; + int ret; + + if (CMIsNullValue(data) || CMIsNullObject(data.value.array)) { + CMSetStatus(s, CMPI_RC_ERR_INVALID_PARAMETER); + CU_DEBUG("Method parameter is NULL"); + return 0; + } + + for (i = 0; i < count; i++) { + CMPIData item; + CMPIInstance *inst; + + item = CMGetArrayElementAt(data.value.array, i, NULL); + + ret = parse_eo_inst_arg(&inst, + broker, + item.value.string, + ns, + s); + + if (ret != 1) { + CU_DEBUG("Parsing argument type %d failed", item.type); + return 0; + } + + CMSetArrayElementAt(*array, i, + (CMPIValue *)&inst, + CMPI_instance); + } + + return 1; +} + +static int parse_eo_param(CMPIArgs **args, + const CMPIBroker *broker, + CMPIData data, + CMPIType type, + const char *arg_name, + const char *ns, + CMPIStatus *s) +{ + CMPIArray *new_array; + CMPIInstance *inst; + int ret; + int count; + + if (type == CMPI_instance) { + ret = parse_eo_inst_arg(&inst, + broker, + data.value.string, + ns, + s); + + if (ret != 1) + return 0; + + *s = CMAddArg(*args, + arg_name, + &(inst), + CMPI_instance); + } else if (type == CMPI_instanceA) { + count = CMGetArrayCount(data.value.array, NULL); + + new_array = CMNewArray(broker, count, CMPI_instance, s); + + ret = parse_eo_array(&new_array, + broker, + data, + ns, + count, + s); + if (ret != 1) + return 0; + + *s = CMAddArg(*args, + arg_name, + &(new_array), + CMPI_instanceA); + } else { + CMSetStatus(s, CMPI_RC_ERR_FAILED); + CU_DEBUG("Unable to parse argument type %d", type); + return 0; + } + + if (s->rc != CMPI_RC_OK) { + CU_DEBUG("Unable to update method argument"); + return 0; + } + + return 1; +} static int validate_arg_type(struct method_arg *arg, const CMPIArgs *args,
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@us.ibm.com
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@linux.vnet.ibm.com
KR> Yes, since modify args. This is the new arg element that will KR> eventually be returned. Er, the rest of the function only seems to call CMAddArg(*args, ...), which doesn't necessitate a double pointer. Unless I missed something like: *args = CMNewArgs(...); then I think you're fine. KR> However, based on the suggestion above, I think things can be KR> reworked so that we won't need the double-pointer. Even better :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
KR> Yes, since modify args. This is the new arg element that will KR> eventually be returned.
Er, the rest of the function only seems to call CMAddArg(*args, ...), which doesn't necessitate a double pointer. Unless I missed something like:
*args = CMNewArgs(...);
then I think you're fine.
KR> However, based on the suggestion above, I think things can be KR> reworked so that we won't need the double-pointer.
Even better :)
Hmm - I didn't end up fixing this. The call is: +static int parse_eo_param(CMPIArgs **args, + const CMPIBroker *broker, + CMPIData data, + CMPIType type, + const char *arg_name, + const char *ns, + CMPIStatus *s) +{ parse_eo_param() updates the args list using CMAddArg(). parse_eo_param() is called by validate_arg_type(). validate_arg_type() gets called on every arg in the original arg list. So, to remove the double pointer, you'd either need to pass the value up from parse_eo_param() to validate_arg_type() and then up again, or you can just pass the arg list down and modify it as you go. I took a look at passing the value up, but dealing with a CMPIValue arg didn't seem as clean. I'm definitely open to suggestions here though, because I think this is awkward as it is. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
Kaitlin Rupert wrote:
Dan Smith wrote:
KR> Yes, since modify args. This is the new arg element that will KR> eventually be returned.
Er, the rest of the function only seems to call CMAddArg(*args, ...), which doesn't necessitate a double pointer. Unless I missed something like: *args = CMNewArgs(...);
then I think you're fine.
KR> However, based on the suggestion above, I think things can be KR> reworked so that we won't need the double-pointer.
Even better :)
Hmm - I didn't end up fixing this. The call is:
+static int parse_eo_param(CMPIArgs **args, + const CMPIBroker *broker, + CMPIData data, + CMPIType type, + const char *arg_name, + const char *ns, + CMPIStatus *s) +{
parse_eo_param() updates the args list using CMAddArg(). parse_eo_param() is called by validate_arg_type(). validate_arg_type() gets called on every arg in the original arg list.
So, to remove the double pointer, you'd either need to pass the value up from parse_eo_param() to validate_arg_type() and then up again, or you can just pass the arg list down and modify it as you go.
I took a look at passing the value up, but dealing with a CMPIValue arg didn't seem as clean. I'm definitely open to suggestions here though, because I think this is awkward as it is.
Ah, my mistake on the double-pointer here. For some reason, I was thinking we'd need the double-pointer so that we could update the arg list. But we only need to update what the pointer points to, so you're right - we can remove the double pointer. I'll send out an update. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
# HG changeset patch # User Kaitlin Rupert <karupert@us.ibm.com> # Date 1201551293 28800 # Node ID 21c3975da22cbea8218aca2923cfd68abf293a97 # Parent a6352980c902f4a0e3d60e97b4c2c9bc6c594674 [CU] Add parse_inst_arg() to validate_args() call in std_invokemethod. Signed-off-by: Kaitlin Rupert <karupert@us.ibm.com> diff -r a6352980c902 -r 21c3975da22c std_invokemethod.c --- a/std_invokemethod.c Mon Jan 28 12:14:52 2008 -0800 +++ b/std_invokemethod.c Mon Jan 28 12:14:53 2008 -0800 @@ -180,9 +180,14 @@ static int parse_eo_param(CMPIArgs **arg static int validate_arg_type(struct method_arg *arg, const CMPIArgs *args, + const CMPIBroker *broker, + const char *ns, + CMPIArgs **new_args, CMPIStatus *s) { CMPIData argdata; + CMPIType type; + int ret; argdata = CMGetArg(args, arg->name, s); if ((s->rc != CMPI_RC_OK) || (CMIsNullValue(argdata))) { @@ -193,10 +198,33 @@ static int validate_arg_type(struct meth } if (argdata.type != arg->type) { - CMSetStatus(s, CMPI_RC_ERR_TYPE_MISMATCH); - CU_DEBUG("Method parameter `%s' type check failed", - arg->name); - return 0; + if (check_for_eo(argdata.type, arg->type, &type) == 1) { + ret = parse_eo_param(new_args, + broker, + argdata, + type, + arg->name, + ns, + s); + + if (ret != 1) + return 0; + } else { + CMSetStatus(s, CMPI_RC_ERR_TYPE_MISMATCH); + CU_DEBUG("Method parameter `%s' type check failed", + arg->name); + return 0; + } + } else { + *s = CMAddArg(*new_args, + arg->name, + &(argdata.value), + argdata.type); + + if (s->rc != CMPI_RC_OK) { + CU_DEBUG("Unable to update method argument"); + return 0; + } } CU_DEBUG("Method parameter `%s' validated type 0x%x", @@ -207,19 +235,31 @@ static int validate_arg_type(struct meth } static int validate_args(struct method_handler *h, - const CMPIArgs *args, + const CMPIArgs **args, + const CMPIObjectPath *ref, + const CMPIBroker *broker, CMPIStatus *s) { + CMPIArgs *new_args; int i; + + new_args = CMNewArgs(broker, s); for (i = 0; h->args[i].name; i++) { int ret; struct method_arg *arg = &h->args[i]; - ret = validate_arg_type(arg, args, s); + ret = validate_arg_type(arg, + *args, + broker, + NAMESPACE(ref), + &new_args, + s); if (!ret) return 0; } + + *args = new_args; return 1; } @@ -254,7 +294,11 @@ CMPIStatus _std_invokemethod(CMPIMethodM goto exit; } - ret = validate_args(h, argsin, &s); + ret = validate_args(h, + &argsin, + reference, + ctx->broker, + &s); if (!ret) goto exit;
participants (2)
-
Dan Smith -
Kaitlin Rupert