[PATCH] [RFC] [CU] Turn std_indication's awesome knob to eleven
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1202420200 18000 # Node ID 7feaabeb36cea13611d6dc045cd2bd946528032b # Parent 60ecfe23d68caaa9dda1f115289f168ea8f4a297 [RFC] [CU] Turn std_indication's awesome knob to eleven This is the prototype of a new std_indication that will handle Enable/DisableIndications and Activate/DeactivateFilter correctly, even on a per-indication basis when one provider handles more than one indication. Those functions will also now have default_foo equivalents in std_indication, as many providers (especially raise-style ones) will have no special needs there. The hope is that this will *require* minimal change from existing Indicatio providers, but *allow* for code reduction in most, and of course new providers should try to utilize this. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 60ecfe23d68c -r 7feaabeb36ce instance_util.c --- a/instance_util.c Tue Feb 05 16:01:32 2008 -0500 +++ b/instance_util.c Thu Feb 07 16:36:40 2008 -0500 @@ -251,6 +251,27 @@ CMPIInstance *cu_dup_instance(const CMPI out: return dest; +} + +char *classname_from_inst(const CMPIBroker *broker, + CMPIInstance *inst, + CMPIStatus *s) +{ + char *ret = NULL; + + CMPIObjectPath *ref; + ref = CMGetObjectPath(inst, s); + if ((s->rc != CMPI_RC_OK) || CMIsNullObject(ref)) { + cu_statusf(broker, s, + CMPI_RC_ERR_FAILED, + "Could not get objectpath from instance"); + goto out; + } + + ret = strdup(CLASSNAME(ref)); + + out: + return ret; } /* diff -r 60ecfe23d68c -r 7feaabeb36ce libcmpiutil.h --- a/libcmpiutil.h Tue Feb 05 16:01:32 2008 -0500 +++ b/libcmpiutil.h Thu Feb 07 16:36:40 2008 -0500 @@ -413,6 +413,19 @@ CMPIStatus cu_validate_ref(const CMPIBro CMPIStatus cu_validate_ref(const CMPIBroker *broker, const CMPIObjectPath *ref, const CMPIInstance *inst); + +/** + * Returns the classname from an instance without forcing user to get + * ObjectPath first. + * + * @param broker A pointer to the current broker + * @param inst Instance to examine + * @param s An out pointer for returning status if error occurs + * @returns Classname of instance (must be freed), NULL on failure + */ +char *classname_from_inst(const CMPIBroker *broker, + CMPIInstance *inst, + CMPIStatus *s); #define DEFAULT_EIN(pn) \ static CMPIStatus pn##EnumInstanceNames(CMPIInstanceMI *self, \ diff -r 60ecfe23d68c -r 7feaabeb36ce std_indication.c --- a/std_indication.c Tue Feb 05 16:01:32 2008 -0500 +++ b/std_indication.c Thu Feb 07 16:36:40 2008 -0500 @@ -32,6 +32,45 @@ #include "std_indication.h" +static struct std_ind_state *get_ind_state(struct std_ind_state **list, + char *ind_name) +{ + int i; + struct std_ind_state *state = NULL; + + for (i = 0; list[i] != NULL; i++) { + if (STREQC((list[i])->ind_name, ind_name)) { + state = list[i]; + break; + } + } + + if (state == NULL) + CU_DEBUG("get_ind_state: failed to find %s", ind_name); + + return state; +} + +static bool is_ind_enabled(struct std_indication_ctx *ctx, + char *ind_name, + CMPIStatus *s) +{ + bool ret = false; + struct std_ind_state *state; + + state = get_ind_state(ctx->ind_states, ind_name); + if (state == NULL) { + cu_statusf(ctx->brkr, s, + CMPI_RC_ERR_FAILED, + "No std_ind_state for %s", ind_name); + goto out; + } + + ret = state->enabled; + out: + return ret; +} + static CMPIStatus trigger(struct std_indication_ctx *ctx, const CMPIContext *context) { @@ -61,20 +100,85 @@ static CMPIStatus raise(struct std_indic const CMPIContext *context, const CMPIArgs *argsin) { + bool enabled; CMPIInstance *inst; - - if (!ctx->enabled) { - CU_DEBUG("Indication disabled, not raising."); - return (CMPIStatus) {CMPI_RC_OK, NULL}; - } - - if (cu_get_inst_arg(argsin, "Indication", &inst) != CMPI_RC_OK) - return (CMPIStatus){CMPI_RC_ERR_FAILED, NULL}; - + CMPIStatus s = {CMPI_RC_OK, NULL}; + char *ind_name = NULL; + + if (cu_get_inst_arg(argsin, "Indication", &inst) != CMPI_RC_OK) { + cu_statusf(ctx->brkr, &s, + CMPI_RC_ERR_FAILED, + "Could not get indication to raise"); + goto out; + } + + ind_name = classname_from_inst(ctx->brkr, inst, &s); + if (s.rc != CMPI_RC_OK || ind_name == NULL) { + cu_statusf(ctx->brkr, &s, + CMPI_RC_ERR_FAILED, + "Couldn't get indication name for enable check."); + } + + enabled = is_ind_enabled(ctx, ind_name, &s); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Problem checking enabled: '%s'", CMGetCharPtr(s.msg)); + goto out; + } + if (!enabled) { + CU_DEBUG("%s disabled, not raising", ind_name); + goto out; + } + + CU_DEBUG("Calling appropriate raise function"); if (ctx->handler->raise_fn == NULL) - return default_raise(ctx->brkr, context, inst); - - return ctx->handler->raise_fn(ctx->brkr, context, inst); + s = default_raise(ctx->brkr, context, inst); + else + s = ctx->handler->raise_fn(ctx->brkr, context, inst); + + out: + free(ind_name); + return s; +} + +CMPIStatus stdi_set_ind_state_prop(struct std_indication_ctx *ctx, + char *ind_name, + int prop, + bool value) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + struct std_ind_state *state; + + CU_DEBUG("In stdi_set_ind_state_prop"); + + state = get_ind_state(ctx->ind_states, ind_name); + if (state == NULL) { + CU_DEBUG("state == NULL, exiting"); + cu_statusf(ctx->brkr, &s, + CMPI_RC_ERR_FAILED, + "Provider has no indication '%s'", ind_name); + goto out; + } + + CU_DEBUG("entering switch"); + switch (prop) { + case STDI_PROP_FILTER: + state->filter_active = value; + CU_DEBUG("Filter: %s set to %d", ind_name, value); + break; + case STDI_PROP_ENABLE: + state->enabled = value; + CU_DEBUG("Enable: %s set to %d", ind_name, value); + break; + default: + CU_DEBUG("default case"); + cu_statusf(ctx->brkr, &s, + CMPI_RC_ERR_FAILED, + "Invalid indication state property: %d", prop); + goto out; + } + + out: + return s; } CMPIStatus stdi_handler(CMPIMethodMI *self, diff -r 60ecfe23d68c -r 7feaabeb36ce std_indication.h --- a/std_indication.h Tue Feb 05 16:01:32 2008 -0500 +++ b/std_indication.h Thu Feb 07 16:36:40 2008 -0500 @@ -58,22 +58,36 @@ typedef CMPIStatus (*raise_indication_t) typedef CMPIStatus (*trigger_indication_t)(const CMPIContext *ctx); +enum {STDI_PROP_FILTER, + STDI_PROP_ENABLE}; + struct std_indication_handler { raise_indication_t raise_fn; trigger_indication_t trigger_fn; }; +struct std_ind_state { + char *ind_name; + bool filter_active; + bool enabled; +}; + struct std_indication_ctx { const CMPIBroker *brkr; struct std_indication_handler *handler; - bool enabled; + struct std_ind_state **ind_states; }; -#define STDI_IndicationMIStub(pfx, pn, _broker, hook, _handler) \ +CMPIStatus stdi_set_ind_state_prop(struct std_indication_ctx *ctx, + char *ind_name, + int prop, + bool state); + +#define STDI_IndicationMIStub(pfx, pn, _broker, hook, _handler, states) \ static struct std_indication_ctx _ctx = { \ .brkr = NULL, \ .handler = _handler, \ - .enabled = false, \ + .ind_states = states, \ }; \ \ static CMPIIndicationMIFT indMIFT__ = { \
Jay Gagnon wrote:
diff -r 60ecfe23d68c -r 7feaabeb36ce instance_util.c --- a/instance_util.c Tue Feb 05 16:01:32 2008 -0500 +++ b/instance_util.c Thu Feb 07 16:36:40 2008 -0500 @@ -251,6 +251,27 @@ CMPIInstance *cu_dup_instance(const CMPI
out: return dest; +} + +char *classname_from_inst(const CMPIBroker *broker, + CMPIInstance *inst, + CMPIStatus *s) +{ + char *ret = NULL; + + CMPIObjectPath *ref; + ref = CMGetObjectPath(inst, s); + if ((s->rc != CMPI_RC_OK) || CMIsNullObject(ref)) { + cu_statusf(broker, s, + CMPI_RC_ERR_FAILED, + "Could not get objectpath from instance"); + goto out; + } + + ret = strdup(CLASSNAME(ref)); + + out: + return ret; }
/* diff -r 60ecfe23d68c -r 7feaabeb36ce libcmpiutil.h --- a/libcmpiutil.h Tue Feb 05 16:01:32 2008 -0500 +++ b/libcmpiutil.h Thu Feb 07 16:36:40 2008 -0500 @@ -413,6 +413,19 @@ CMPIStatus cu_validate_ref(const CMPIBro CMPIStatus cu_validate_ref(const CMPIBroker *broker, const CMPIObjectPath *ref, const CMPIInstance *inst); + +/** + * Returns the classname from an instance without forcing user to get + * ObjectPath first. + * + * @param broker A pointer to the current broker + * @param inst Instance to examine + * @param s An out pointer for returning status if error occurs + * @returns Classname of instance (must be freed), NULL on failure + */ +char *classname_from_inst(const CMPIBroker *broker, + CMPIInstance *inst, + CMPIStatus *s);
#define DEFAULT_EIN(pn) \ static CMPIStatus pn##EnumInstanceNames(CMPIInstanceMI *self, \
Please let me play the PatchCO today - is this really part of the indication patch ;) ? -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
Heidi Eckhart wrote:
Jay Gagnon wrote:
diff -r 60ecfe23d68c -r 7feaabeb36ce instance_util.c --- a/instance_util.c Tue Feb 05 16:01:32 2008 -0500 +++ b/instance_util.c Thu Feb 07 16:36:40 2008 -0500 @@ -251,6 +251,27 @@ CMPIInstance *cu_dup_instance(const CMPI
out: return dest; +} + +char *classname_from_inst(const CMPIBroker *broker, + CMPIInstance *inst, + CMPIStatus *s) +{ + char *ret = NULL; + + CMPIObjectPath *ref; + ref = CMGetObjectPath(inst, s); + if ((s->rc != CMPI_RC_OK) || CMIsNullObject(ref)) { + cu_statusf(broker, s, + CMPI_RC_ERR_FAILED, + "Could not get objectpath from instance"); + goto out; + } + + ret = strdup(CLASSNAME(ref)); + + out: + return ret; }
/* diff -r 60ecfe23d68c -r 7feaabeb36ce libcmpiutil.h --- a/libcmpiutil.h Tue Feb 05 16:01:32 2008 -0500 +++ b/libcmpiutil.h Thu Feb 07 16:36:40 2008 -0500 @@ -413,6 +413,19 @@ CMPIStatus cu_validate_ref(const CMPIBro CMPIStatus cu_validate_ref(const CMPIBroker *broker, const CMPIObjectPath *ref, const CMPIInstance *inst); + +/** + * Returns the classname from an instance without forcing user to get + * ObjectPath first. + * + * @param broker A pointer to the current broker + * @param inst Instance to examine + * @param s An out pointer for returning status if error occurs + * @returns Classname of instance (must be freed), NULL on failure + */ +char *classname_from_inst(const CMPIBroker *broker, + CMPIInstance *inst, + CMPIStatus *s);
#define DEFAULT_EIN(pn) \ static CMPIStatus pn##EnumInstanceNames(CMPIInstanceMI *self, \
Please let me play the PatchCO today - is this really part of the indication patch ;) ?
Touche! :) No, you're right, it doesn't really belong. I unfortunately sent these two out in a bit of a rush right before I left yesterday, so I spent all of 30 seconds scanning for patch quality. I'm sure there are a bunch of little things, and I promise they'll be fixed in the next version, but for the most part I'm just looking for a sanity check here. Just want to make sure that I'm not doing something really crazy before I invest much more time into this. -- -Jay
JG> +char *classname_from_inst(const CMPIBroker *broker, JG> + CMPIInstance *inst, JG> + CMPIStatus *s) JG> +{ JG> + char *ret = NULL; JG> + JG> + CMPIObjectPath *ref; JG> + ref = CMGetObjectPath(inst, s); JG> + if ((s->rc != CMPI_RC_OK) || CMIsNullObject(ref)) { JG> + cu_statusf(broker, s, JG> + CMPI_RC_ERR_FAILED, JG> + "Could not get objectpath from instance"); JG> + goto out; JG> + } JG> + JG> + ret = strdup(CLASSNAME(ref)); JG> + JG> + out: JG> + return ret; JG> } This could probably be a distinct patch as well. Also, why strdup the result of CLASSNAME()? It's CIMOM-managed memory, and most of the other libcu functions have been modified to avoid dynamic memory where possible. The return value should be const char * after this change. Also, why not just signal your single error case by returning NULL? That way you can avoid having to pass in the broker and the status pointers and the function becomes a bit cleaner. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> +char *classname_from_inst(const CMPIBroker *broker, JG> + CMPIInstance *inst, JG> + CMPIStatus *s) JG> +{ JG> + char *ret = NULL; JG> + JG> + CMPIObjectPath *ref; JG> + ref = CMGetObjectPath(inst, s); JG> + if ((s->rc != CMPI_RC_OK) || CMIsNullObject(ref)) { JG> + cu_statusf(broker, s, JG> + CMPI_RC_ERR_FAILED, JG> + "Could not get objectpath from instance"); JG> + goto out; JG> + } JG> + JG> + ret = strdup(CLASSNAME(ref)); JG> + JG> + out: JG> + return ret; JG> }
This could probably be a distinct patch as well.
Also, why strdup the result of CLASSNAME()? It's CIMOM-managed memory, and most of the other libcu functions have been modified to avoid dynamic memory where possible. The return value should be const char * after this change.
As this illustrates, I'm never really sure whether or not strdup is the right thing to do. It would appear that it's unnecessary here, so I'll take it out.
Also, why not just signal your single error case by returning NULL? That way you can avoid having to pass in the broker and the status pointers and the function becomes a bit cleaner.
Probably just my natural tendency to over-complicate. I'll fix that as well. -- -Jay
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon