[Openais] A patch for configuration change of AMF
Steven Dake
sdake at mvista.com
Mon Sep 13 15:17:14 PDT 2004
Sakai-san,
Some comments inline on the patch. Could you address them and post
another patch?
On Sun, 2004-09-12 at 07:54, SAKAI MIYOTAKA wrote:
> Steave ,
>
> I made a patch for configuration change .
> This patch is also corresponding to a certain node joining .
>
> Could you review the patch attached to this message ?
>
> I think this path works well .
>
> If you accept this patch ,next step is that each separate node has
> Active unit ,when joining each other.
> For example ,
> Node A has the unit that is Active .
> Node B has the unit that is Active .
> After that these nodes joining each other .
>
> In that case this patch doesn't work .
>
> SAKAI MIYOTAKA wrote:
>
> >Steve ,
> >
> >I made a patch for configuration change when a certain node leaves.
> >
> >Could you review the patch attached to this message ?
> >
> >I think review points are followings
> >
> >- The basic frame of configuration change for AMF.
> >- Adding source_addr to saAmfComponent as a member.
> >
> >thanks
> >- Miyotaka sakai
> >
> >
>
> ______________________________________________________________________
> diff -uNr openais.2004-09-09.21.30/exec/amf.c openais/exec/amf.c
> --- openais.2004-09-09.21.30/exec/amf.c 2004-09-10 13:30:02.000000000 +0900
> +++ openais/exec/amf.c 2004-09-12 22:49:09.000000000 +0900
> @@ -36,6 +36,7 @@
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <netinet/in.h>
> +#include <arpa/inet.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <stdlib.h>
> @@ -142,6 +143,10 @@
> static int activeServiceUnitsCount (
> struct saAmfGroup *saAmfGroup);
>
> +static void componentRegister( struct saAmfComponent *component );
> +
^^ can you match the coding style in the rest of the code?
try instead
static void componentRegister (struct saAmfComponent *component);
in fact, feel welcome to rename componentRegister to
component_register. I'd like to get rid of all of the uppercase
function names in the executive.
> +static void componentUnregister ( struct saAmfComponent *component );
same as above
> +
> #ifdef COMPILE_OUT
> static void enumerateComponents (
> void (*function)(struct saAmfComponent *, void *data),
> @@ -223,6 +228,18 @@
>
> static int amfExecutiveInitialize (void);
>
> +static void amf_confchg_nleave ( struct saAmfComponent *component ,void *data );
> +
> +static void amf_confchg_njoin ( struct saAmfComponent *component ,void *data );
> +
> +static void amf_confchg_nsync ( struct saAmfComponent *component ,void *data );
> +
> +static int amf_confchg_fn (
> + struct sockaddr_in *member_list, int member_list_entries,
> + struct sockaddr_in *left_list, int left_list_entries,
> + struct sockaddr_in *joined_list, int joined_list_entries);
> +
> +
> static int message_handler_req_exec_amf_componentregister (void *message, struct in_addr source_addr);
>
> static int message_handler_req_exec_amf_componentunregister (void *message, struct in_addr source_addr);
> @@ -371,7 +388,7 @@
> .libais_handlers_count = sizeof (amf_libais_handlers) / sizeof (struct libais_handler),
> .aisexec_handler_fns = amf_aisexec_handler_fns,
> .aisexec_handler_fns_count = sizeof (amf_aisexec_handler_fns) / sizeof (int (*)),
> - .confchg_fn = 0,
> + .confchg_fn = amf_confchg_fn,
> .libais_init_fn = message_handler_req_amf_init,
> .libais_exit_fn = amf_exit_fn,
> .aisexec_init_fn = amfExecutiveInitialize
> @@ -488,8 +505,41 @@
> gmi_mcast (&aisexec_groupname, iovecs, 1, GMI_PRIO_MED);
> }
>
> -#ifdef COMPILE_OUT
> -This should be used for a partition I think
> +void componentRegister (
> + struct saAmfComponent *component)
> +{
> + struct req_exec_amf_componentregister req_exec_amf_componentregister;
> + struct iovec iovecs[2];
> +
> + /*
> + * This only works on local components
> + */
> + if (component == 0 || component->local != 1) {
> + return;
> + }
> + log_printf (LOG_LEVEL_DEBUG, "componentRegister: registering component %s\n",
> + getSaNameT (&component->name));
> +// component->probableCause = SA_AMF_NOT_RESPONDING;
> +
> + req_exec_amf_componentregister.header.size = sizeof (struct req_exec_amf_componentregister);
> + req_exec_amf_componentregister.header.id = MESSAGE_REQ_EXEC_AMF_COMPONENTREGISTER;
> +
> + req_exec_amf_componentregister.source.conn_info = 0;
> + req_exec_amf_componentregister.source.in_addr.s_addr = 0;
> +
> + memset (&req_exec_amf_componentregister.req_lib_amf_componentregister,
> + 0, sizeof (struct req_lib_amf_componentregister));
> + memcpy (&req_exec_amf_componentregister.req_lib_amf_componentregister.compName,
> + &component->name,
> + sizeof (SaNameT));
> +
> + iovecs[0].iov_base = (char *)&req_exec_amf_componentregister;
> + iovecs[0].iov_len = sizeof (req_exec_amf_componentregister);
> +
> + gmi_mcast (&aisexec_groupname, iovecs, 1, GMI_PRIO_HIGH);
> +}
In the case of a partition or merge, you want to use GMI_PRIO_RECOVERY.
You may have to pass a variable to component_register and
component_unregister to specify the priority of th message.
> +
> +// This should be used for a partition I think
> // This should be used for partition changes
This says the same thing twice.
No C++ style comments please. I know they are in there now, but they
need to be removed as we go forward.
Stick with:
/*
*
*/
> void enumerateComponents (
> void (*function)(struct saAmfComponent *, void *data),
> @@ -503,7 +553,6 @@
> struct saAmfUnit *AmfUnit;
> struct saAmfComponent *AmfComponent;
>
> -
> /*
> * Search all groups
> */
> @@ -539,7 +588,6 @@
> }
> }
> }
> -#endif
>
> int activeServiceUnitsCount (struct saAmfGroup *saAmfGroup) {
> struct saAmfUnit *saAmfUnit;
> @@ -568,8 +616,7 @@
>
> saAmfComponent = list_entry (saAmfComponentList,
> struct saAmfComponent, saAmfComponentList);
> -
> - if (saAmfComponent->currentHAState != SA_AMF_ACTIVE) {
> + if (saAmfComponent->/*currentHAState*/newHAState != SA_AMF_ACTIVE) {
> thisServiceUnitActive = 0;
>
this change looks really fishy. Could you explain why you need to use
the newhastate instead of the current ha state? The new ha state
shouldn't have a valid value for the case of enabledunlockedcompleted.
> }
> }
> @@ -1623,6 +1670,79 @@
> return (0);
> }
>
> +void amf_confchg_nleave( struct saAmfComponent *component ,void *data ) {
> +
> + struct in_addr *source_addr;
> + source_addr = ( struct in_addr *) data;
> +
> +// printf( "amf_confchg_nleave(%s)\n" ,(char *)inet_ntoa(*source_addr));
> +// printf( " (%s)\n" ,(char *)inet_ntoa(component->source_addr));
> +
there is debug printing aparatus, perhaps you could use this instead of
commenting out the printfs?
> + if ( memcmp( &(component->source_addr) ,source_addr ,sizeof(*source_addr)) ) {
> + return;
> + }
> +
coding style is (memcmp (c1, c2, sizeof (struct in_addr));
> + component -> local = 1;
could you match the coding style in the code:
component->local = 1;
> + componentUnregister( component );
> +
> + return;
> +}
> +
> +void amf_confchg_njoin ( struct saAmfComponent *component ,void *data ) {
> +
> + if ( memcmp( &(component->source_addr),&(this_ip.sin_addr.s_addr),sizeof(component->source_addr)) ) {
> + return;
> + }
> +
> + componentRegister( component );
> + return;
> +}
> +
> +void amf_confchg_nsync ( struct saAmfComponent *component ,void *data ) {
> +
> + if ( memcmp( &(component->source_addr),&(this_ip.sin_addr.s_addr),sizeof(component->source_addr)) ) {
> + return;
> + }
> +
> + readinessStateSetCluster( component ,component->currentReadinessState );
> + haStateSetCluster ( component ,component->currentHAState );
I'm a little surprised this would work directly without additional
state. Have you tested several cases? Ie: trying testamf5,6 then 1,2
and 56, then 1,2 etc?
> +
> + return;
> +}
> +
> +int amf_confchg_fn (
> + struct sockaddr_in *member_list, int member_list_entries,
> + struct sockaddr_in *left_list, int left_list_entries,
> + struct sockaddr_in *joined_list, int joined_list_entries){
> +
> + int i;
> + int size;
> + void *data;
> +
> + /*
> + * If node join , component register
> + */
> + if ( joined_list_entries > 0 ) {
> + enumerateComponents ( amf_confchg_njoin ,NULL );
> + enumerateComponents ( amf_confchg_nsync ,NULL );
> + }
> +
> + /*
> + * If node leave , component unregister
> + */
> + size = sizeof((member_list->sin_addr));
> + if ( memcmp( &(member_list->sin_addr), &(this_ip.sin_addr.s_addr) ,size ) ){
> + return(0);
> + }
> +
> + for ( i = 0; i<left_list_entries ; i++ ) {
> + data = ( void *)&(left_list->sin_addr); left_list ++;
> + enumerateComponents ( amf_confchg_nleave ,data );
> + }
> +
the above construct is a little complicated with the pointer addition.
could you use instead somethign like:
enumerateComponent (amf_confchg_nleave, left_list[i].sin_addr);
> + return (0);
> +}
> +
> int amf_exit_fn (struct conn_info *conn_info)
> {
> /*
> @@ -1647,7 +1767,6 @@
> return (0);
> }
>
> -
> static int message_handler_req_exec_amf_componentregister (void *message, struct in_addr source_addr)
> {
> struct req_exec_amf_componentregister *req_exec_amf_componentregister = (struct req_exec_amf_componentregister *)message;
> @@ -1700,6 +1819,7 @@
> component->local = 0;
> component->registered = 1;
> component->conn_info = req_exec_amf_componentregister->source.conn_info;
> + component->source_addr = source_addr;
> component->currentReadinessState = SA_AMF_OUT_OF_SERVICE;
> component->newReadinessState = SA_AMF_OUT_OF_SERVICE;
> component->currentHAState = SA_AMF_QUIESCED;
> @@ -1916,6 +2036,7 @@
> req_exec_amf_readinessstateset->readinessState);
>
> component->currentReadinessState = req_exec_amf_readinessstateset->readinessState;
> + component->newReadinessState = component->currentReadinessState;
> dsm (component);
> }
>
> @@ -1939,6 +2060,7 @@
> getSaNameT (&component->name),
> req_exec_amf_hastateset->haState);
> component->currentHAState = req_exec_amf_hastateset->haState;
> + component->newHAState = component->currentHAState;
> dsm (component);
> }
>
> diff -uNr openais.2004-09-09.21.30/exec/parse.h openais/exec/parse.h
> --- openais.2004-09-09.21.30/exec/parse.h 2004-09-10 13:30:02.000000000 +0900
> +++ openais/exec/parse.h 2004-09-11 10:42:04.000000000 +0900
> @@ -130,6 +130,7 @@
> int registered;
> int local;
> struct conn_info *conn_info;
> + struct in_addr source_addr;
> SaNameT name;
> SaAmfReadinessStateT currentReadinessState;
> SaAmfReadinessStateT newReadinessState;
Sakai-san
looks good. If we can fix up the coding style issues and the one
strange code segment, we are ready to merge.
Thanks
-steve
More information about the Openais
mailing list