[PATCH] [RFC] Changes required to work with new std_indication. The trigger changes were very light, which was nice
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1202420324 18000 # Node ID a82dd87a3830eadda678b88bf4bfcfd4f8cb1ca4 # Parent c7dd4a8358a187a3469c3d8a177950625898a227 [RFC] Changes required to work with new std_indication. The trigger changes were very light, which was nice. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r c7dd4a8358a1 -r a82dd87a3830 src/Virt_ComputerSystemIndication.c --- a/src/Virt_ComputerSystemIndication.c Wed Feb 06 07:00:00 2008 -0800 +++ b/src/Virt_ComputerSystemIndication.c Thu Feb 07 16:38:44 2008 -0500 @@ -488,7 +488,8 @@ STDI_IndicationMIStub(, Virt_ComputerSystemIndication, _BROKER, libvirt_cim_init(), - &csi); + &csi, + NULL); /* * Local Variables: diff -r c7dd4a8358a1 -r a82dd87a3830 src/Virt_ComputerSystemMigrationIndication.c --- a/src/Virt_ComputerSystemMigrationIndication.c Wed Feb 06 07:00:00 2008 -0800 +++ b/src/Virt_ComputerSystemMigrationIndication.c Thu Feb 07 16:38:44 2008 -0500 @@ -58,7 +58,13 @@ static CMPIStatus ActivateFilter(CMPIInd CMPIBoolean first) { CMPIStatus s = {CMPI_RC_OK, NULL}; + struct std_indication_ctx *_ctx; + _ctx = (struct std_indication_ctx *)mi->hdl; + s = stdi_set_ind_state_prop(_ctx, + "Xen_ComputerSystemMigrationIndication", + STDI_PROP_FILTER, + true); return s; } @@ -69,16 +75,31 @@ static CMPIStatus DeActivateFilter(CMPII const CMPIObjectPath* op, CMPIBoolean last) { - return (CMPIStatus){CMPI_RC_OK, NULL}; + CMPIStatus s = {CMPI_RC_OK, NULL}; + struct std_indication_ctx *_ctx; + + _ctx = (struct std_indication_ctx *)mi->hdl; + s = stdi_set_ind_state_prop(_ctx, + "Xen_ComputerSystemMigrationIndication", + STDI_PROP_FILTER, + true); + return s; } static _EI_RTYPE EnableIndications(CMPIIndicationMI* mi, const CMPIContext *ctx) { + CMPIStatus s = {CMPI_RC_OK, NULL}; struct std_indication_ctx *_ctx; _ctx = (struct std_indication_ctx *)mi->hdl; - _ctx->enabled = true; - CU_DEBUG("ComputerSystemModifiedIndication enabled"); + + CU_DEBUG("New enable. Cross your fingers"); + s = stdi_set_ind_state_prop(_ctx, + "Xen_ComputerSystemMigrationIndication", + STDI_PROP_ENABLE, + true); + + CU_DEBUG("ComputerSystemMigrationIndication enabled"); _EI_RET(); } @@ -86,10 +107,16 @@ static _EI_RTYPE DisableIndications(CMPI static _EI_RTYPE DisableIndications(CMPIIndicationMI* mi, const CMPIContext *ctx) { + CMPIStatus s = {CMPI_RC_OK, NULL}; struct std_indication_ctx *_ctx; _ctx = (struct std_indication_ctx *)mi->hdl; - _ctx->enabled = false; - CU_DEBUG("ComputerSystemModifiedIndication disabled"); + + s = stdi_set_ind_state_prop(_ctx, + "Xen_ComputerSystemMigrationIndication", + STDI_PROP_ENABLE, + false); + + CU_DEBUG("ComputerSystemMigrationIndication disabled"); _EI_RET(); } @@ -97,6 +124,17 @@ static struct std_indication_handler csi static struct std_indication_handler csi = { .raise_fn = NULL, .trigger_fn = NULL, +}; + +static struct std_ind_state migrate = { + .ind_name = "Xen_ComputerSystemMigrationIndication", + .filter_active = false, + .enabled = false, +}; + +static struct std_ind_state *states[] = { + &migrate, + NULL, }; DEFAULT_IND_CLEANUP(); @@ -107,7 +145,8 @@ STDI_IndicationMIStub(, Virt_ComputerSystemMigrationIndication, _BROKER, libvirt_cim_init(), - &csi); + &csi, + states); /* * Local Variables:
Jay Gagnon wrote:
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1202420324 18000 # Node ID a82dd87a3830eadda678b88bf4bfcfd4f8cb1ca4 # Parent c7dd4a8358a187a3469c3d8a177950625898a227 [RFC] Changes required to work with new std_indication. The trigger changes were very light, which was nice.
Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com>
diff -r c7dd4a8358a1 -r a82dd87a3830 src/Virt_ComputerSystemIndication.c --- a/src/Virt_ComputerSystemIndication.c Wed Feb 06 07:00:00 2008 -0800 +++ b/src/Virt_ComputerSystemIndication.c Thu Feb 07 16:38:44 2008 -0500 @@ -488,7 +488,8 @@ STDI_IndicationMIStub(, Virt_ComputerSystemIndication, _BROKER, libvirt_cim_init(), - &csi); + &csi, + NULL);
/* * Local Variables:
I'm interested in the other changes that are necessary to the CSIndication provider to make use of the std_indication reorg - also make use of the states and not setting it to NULL ;).
diff -r c7dd4a8358a1 -r a82dd87a3830 src/Virt_ComputerSystemMigrationIndication.c --- a/src/Virt_ComputerSystemMigrationIndication.c Wed Feb 06 07:00:00 2008 -0800 +++ b/src/Virt_ComputerSystemMigrationIndication.c Thu Feb 07 16:38:44 2008 -0500
If I got this right, then the MigrationIndication provider is currently only switching the migrate.filter_activate and migrate.enabled values. Potentially it could deliver an indication (by the default_raise function), but it can not as the trigger_fn is not implemented and so nothing is monitored, right ? Well, in sum - I like the approach :) - but I would even go farther. I think its possible to write one indication engine (that would become part of libcmpiutil) that handles all the CIM_InstModification, CIM_InstCreation and CIM_InstDeletion indications at once. This engine would be a large part of the CSIndication provider and maybe only the compare functions (dom_in_list, dom_changed) need to stay provider specific. But that's a guess at the moment and is worth to be evaluated (now I hope that you volunteer for this job ;) ). -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
Heidi Eckhart wrote:
Jay Gagnon wrote:
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1202420324 18000 # Node ID a82dd87a3830eadda678b88bf4bfcfd4f8cb1ca4 # Parent c7dd4a8358a187a3469c3d8a177950625898a227 [RFC] Changes required to work with new std_indication. The trigger changes were very light, which was nice.
Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com>
diff -r c7dd4a8358a1 -r a82dd87a3830 src/Virt_ComputerSystemIndication.c --- a/src/Virt_ComputerSystemIndication.c Wed Feb 06 07:00:00 2008 -0800 +++ b/src/Virt_ComputerSystemIndication.c Thu Feb 07 16:38:44 2008 -0500 @@ -488,7 +488,8 @@ STDI_IndicationMIStub(, Virt_ComputerSystemIndication, _BROKER, libvirt_cim_init(), - &csi); + &csi, + NULL);
/* * Local Variables:
I'm interested in the other changes that are necessary to the CSIndication provider to make use of the std_indication reorg - also make use of the states and not setting it to NULL ;). Haha yea it's just set to NULL here in the interest of using the minimal amount of change to not break it. When I have everything else in place I'll make sure CSIndication fully utilizes the new code.
diff -r c7dd4a8358a1 -r a82dd87a3830 src/Virt_ComputerSystemMigrationIndication.c --- a/src/Virt_ComputerSystemMigrationIndication.c Wed Feb 06 07:00:00 2008 -0800 +++ b/src/Virt_ComputerSystemMigrationIndication.c Thu Feb 07 16:38:44 2008 -0500
If I got this right, then the MigrationIndication provider is currently only switching the migrate.filter_activate and migrate.enabled values. Potentially it could deliver an indication (by the default_raise function), but it can not as the trigger_fn is not implemented and so nothing is monitored, right ? That is correct, and that's actually the intended behavior. Attempting to monitor out of band migrations is not a problem I'm terribly interested in solving at the moment, and the raise functionality is tied into by VSMigrationService in a previous patch. The end result is that whenever our providers are asked to do a migration task, VSMigrationService uses MigrationIndication's raise call to send the indication.
Well, in sum - I like the approach :) - but I would even go farther. I think its possible to write one indication engine (that would become part of libcmpiutil) that handles all the CIM_InstModification, CIM_InstCreation and CIM_InstDeletion indications at once. This engine would be a large part of the CSIndication provider and maybe only the compare functions (dom_in_list, dom_changed) need to stay provider specific. But that's a guess at the moment and is worth to be evaluated (now I hope that you volunteer for this job ;) ).
Yes, this will definitely go further. This is basically phase one of three. -- -Jay
I agree with Heidi. I think you've opened the door to take it a little further so that the calls in Enable/Disable and Activate/Deactivate are automated for the simple case. Maybe this is already planned? JG> +static struct std_ind_state migrate = { JG> + .ind_name = "Xen_ComputerSystemMigrationIndication", JG> + .filter_active = false, JG> + .enabled = false, JG> +}; This is a really minor thing, but this definition has some fields that must always be set to false on initialization to avoid badness. Now, since these are static, they are all initialized to zero, which should cover you in the case that someone just doesn't set filter_active and enabled. However, perhaps setting the precedent with something like a constructor macro would be appropriate? #define DECLARE_IND(ident, name) \ static struct std_ind_state ident = { \ .ind_name = name, \ .filter_active = false, \ .enabled = false, \ }; Which would make the above definition look like this: DECLARE_IND(migrate, "Xen_ComputerSystemMigrationIndication"); Thoughts? -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
I agree with Heidi. I think you've opened the door to take it a little further so that the calls in Enable/Disable and Activate/Deactivate are automated for the simple case. Maybe this is already planned?
Yup, those are phase two.
JG> +static struct std_ind_state migrate = { JG> + .ind_name = "Xen_ComputerSystemMigrationIndication", JG> + .filter_active = false, JG> + .enabled = false, JG> +};
This is a really minor thing, but this definition has some fields that must always be set to false on initialization to avoid badness. Now, since these are static, they are all initialized to zero, which should cover you in the case that someone just doesn't set filter_active and enabled. However, perhaps setting the precedent with something like a constructor macro would be appropriate?
#define DECLARE_IND(ident, name) \ static struct std_ind_state ident = { \ .ind_name = name, \ .filter_active = false, \ .enabled = false, \ };
Which would make the above definition look like this:
DECLARE_IND(migrate, "Xen_ComputerSystemMigrationIndication");
Thoughts?
Ooh, I like that. I'll definitely use it. -- -Jay
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon