[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