[Openais] A patch for configuration change of AMF
SAKAI MIYOTAKA
sakai.miyotaka at nttcom.co.jp
Tue Sep 14 07:18:09 PDT 2004
Steve ,
Thank you for your comments .
I am replying inline .
>Sakai-san,
>
>Some comments inline on the patch. Could you address them and post
>another patch?
>
Yes ,I will .
>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.
>
OK .
I 'll be carefull to match the coding style in the rest of the code .
>>+static void componentUnregister ( struct saAmfComponent *component );
>>
>>
>
>same as above
>
OK .
>>+
>> #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.
>
OK. I use GMI_PRIO_RECOVERY .
>>+
>>+// 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.
>
OK . I have to use C style comments .
>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.
>
>
For example ,
After MESSAGE_RES_AMF_CSISETCALLBACK is sent
and by the time message_handler_req_exec_amf_hastateset() is called ,
the currentHAstate (SA_AMF_QUIESED) is different from the
newHAState(SA_AMF_ACTIVE) .
In this sutiation if dsmEnabledUnlockedInServiceCompleted() is called ,
the program calls haStateSetApi( SA_AMF_ACTIVE ),
But in this case , I expect that component becomes SA_AMF_STANDBY.
that is the reason .
>> }
>>@@ -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?
>
OK . I' ll get rid of these debug printf .
>>r) ,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;
>
OK .
>>+ 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?
>
>
Thank you for your advice .
I needed to explain more .
I didn't think this code work all of situation ,either .
But in the simple situation it works and it is easier to test .
I 'll change this code , but give me some time .
>>+
>>+ 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);
>
OK .
>>+ 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.
>
>
Thank you very much for your advice .
- Miyotaka sakai
>Thanks
>-steve
>
>
More information about the Openais
mailing list