[Openais] proposing patches for AMF

Hans Feldt Hans.Feldt at ericsson.com
Mon Sep 25 01:25:27 PDT 2006


Hi,

Some comments:
- Change name of the macro ALIGN_ADDR(addr) to just ALIGN(x) since that 
seems more correct the way the macro is used.

- Change testamf1.c to use sched_get_priority_max() instead of 
hardcoding the limit and using #ifdefs which is ugly.

- Parenthesis around return values is style, don't change that, should 
be consistent with rest of the file since no design rule exist about that.

I have no problem merging these fixes after that. Can you send the new 
diff file as an attachment instead?

However I don't understand how it can work on sparc even with these 
change since the totem interface delivers messages to AMF totally unaligned:

> Sep 25  9:37:28.547505 [amf.c:1703] >message_handler_req_exec_amf_sync_data: rec 568 bytes, type 2, ptr 0x4036a026
> Sep 25  9:37:28.547535 [amf.c:1703] >message_handler_req_exec_amf_sync_data: rec 584 bytes, type 3, ptr 0x4036a263
> Sep 25  9:37:28.547571 [amf.c:1703] >message_handler_req_exec_amf_sync_data: rec 584 bytes, type 3, ptr 0x4036a011

Can sparcs read words at unaligned addresses?

Regards,
Hans


Mathieu Marie wrote:
> Hi,
> 
> This is my first time on this alias.
> 
> I just installed openAIS (trunk) on Solaris and tried to play around 
> with AMF.
> I found and fixed some minor problems that are mentioned below,
> I added my diffs, in case you're interested in incorporating them.
> 
> 
> Here are the problems I found :
> 
> 1- On Solaris/sparc, the amf crashes with a SIGBUS, because
>      the serialize/deserialize methods are not aligned for sparc.
>      (amfutil.c)
>      I fixed it by aligning everything on 8 on sparc. The current fix
>      does not allow to communicate between sparc and non-sparc
>      nodes, because I differentiated the sparc case.
>      To allow them to communicate, the ALIGN_ADDR macro below
>      should be the same in sparc and non-sparc cases.
> 
>      In the same fix, I modified the sa_serialize_SaUint64T to make
>      it similar to the other methods (that method is not currently used 
> in trunk).
> 
> Index: amfutil.c
> ===================================================================
> --- amfutil.c   (revision 1241)
> +++ amfutil.c   (working copy)
> @@ -1067,14 +1067,19 @@
>         return assignment_state_text[state];
>  }
>  
> -#define ALIGN_ADDR(addr) ((addr) + (4 - ((unsigned long)(addr) % 4)))
> +#ifdef __sparc
> +#define ALIGN_ADDR(addr) ((addr) + (8 - ((unsigned long)(addr) % 8)))
> +#else
> +#define ALIGN_ADDR(addr) addr
> +#endif
>  
>  char *amf_serialize_SaNameT (char *buf, int *size, int *offset, SaNameT 
> *name)
>  {
>         char *tmp = buf;
>  
> -       if ((*size - *offset ) < sizeof (SaNameT)) {
> -               *size += sizeof (SaNameT);
> +       if ((*size - *offset ) < ALIGN_ADDR(sizeof (SaNameT))) {
> +               *size += ALIGN_ADDR(sizeof(SaNameT));
> +
>                 tmp = realloc (buf, *size);
>                 if (tmp == NULL) {
>                         openais_exit_error (AIS_DONE_OUT_OF_MEMORY);
> @@ -1083,7 +1088,7 @@
>  
>         memcpy (&tmp[*offset], name, sizeof (SaNameT));
>  
> -       (*offset) += sizeof (SaNameT);
> +       (*offset) += ALIGN_ADDR(sizeof (SaNameT));
>  
>         return tmp;
>  }
> @@ -1098,61 +1103,77 @@
>                 len = 0;
>         }
>  
> -       return amf_serialize_opaque (buf, size, offset, str, len);
> +       return (amf_serialize_opaque (buf, size, offset, str, len));
>  }
>  
>  char *amf_serialize_SaUint32T (char *buf, int *size, int *offset, 
> SaUint32T num)
>  {
>         char *tmp = buf;
> +       int lsize = *size;
>  
> -       if ((*size - *offset ) < sizeof (SaUint32T)) {
> -               *size += sizeof (SaUint32T);
> -               tmp = realloc (buf, *size);
> +       if ((*size - *offset ) < ALIGN_ADDR(sizeof (SaUint32T))) {
> +               lsize += ALIGN_ADDR(sizeof (SaUint32T));
> +               tmp = realloc (buf, lsize);
>                 if (tmp == NULL) {
>                         openais_exit_error (AIS_DONE_OUT_OF_MEMORY);
>                 }
>         }
>  
>         *((SaUint32T *)&tmp[*offset]) = num;
> -       (*offset) += sizeof (SaUint32T);
> +       (*offset) += ALIGN_ADDR(sizeof (SaUint32T));
> +       *size = lsize;
>  
> -       return tmp;
> +       return (tmp);
>  }
>  
> -char *amf_serialize_SaUint64T (char *buf, SaUint64T num)
> +char *amf_serialize_SaUint64T (char *buf, int *size, int *offset, 
> SaUint64T num)
>  {
> -       *((SaUint64T *)buf) = num;
> -       return buf + sizeof (SaUint64T);
> +       char *tmp = buf;
> +       int lsize = *size;
> +
> +       if ((*size - *offset ) < ALIGN_ADDR(sizeof (SaUint64T))) {
> +               lsize += ALIGN_ADDR(sizeof (SaUint64T));
> +               tmp = realloc (buf, lsize);
> +               if (tmp == NULL) {
> +                       openais_exit_error (AIS_DONE_OUT_OF_MEMORY);
> +               }
> +       }
> +
> +       *((SaUint64T *)&tmp[*offset]) = num;
> +       (*offset) += ALIGN_ADDR(sizeof (SaUint64T));
> +       *size = lsize;
> +
> +       return (tmp);
>  }
>  
>  char *amf_serialize_opaque (
>         char *buf, int *size, int *offset, char *src, int cnt)
>  {
> -       unsigned int required_size;
> +
>         char *tmp = buf;
> +       int lsize = *size;
>  
> -       required_size = cnt + sizeof (SaUint32T);
> -
> -       if ((*size - *offset ) < required_size) {
> -               *size += required_size;
> -               tmp = realloc (buf, *size);
> +       if ((*size - *offset ) < (ALIGN_ADDR(sizeof(SaUint32T)) + 
> ALIGN_ADDR(cnt))) {
> +               lsize += (ALIGN_ADDR(sizeof(SaUint32T)) + ALIGN_ADDR(cnt));
> +               tmp = realloc (buf, lsize);
>                 if (tmp == NULL) {
>                         openais_exit_error (AIS_DONE_OUT_OF_MEMORY);
>                 }
>         }
>  
>         *((SaUint32T *)&tmp[*offset]) = cnt;
> -       (*offset) += sizeof (SaUint32T);
> +       (*offset) += ALIGN_ADDR(sizeof (SaUint32T));
>         memcpy (&tmp[*offset], src, cnt);
> -       (*offset) += cnt;
> +       (*offset) += ALIGN_ADDR(cnt);
> +       *size = lsize;
>  
> -       return tmp;
> +       return (tmp);
>  }
>  
>  char *amf_deserialize_SaNameT (char *buf, SaNameT *name)
>  {
>         memcpy (name, buf, sizeof (SaNameT));
> -       return (buf + sizeof (SaNameT));
> +       return (buf + ALIGN_ADDR(sizeof (SaNameT)));
>  }
>  
>  char *amf_deserialize_SaStringT (char *buf, SaStringT *str)
> @@ -1161,7 +1182,7 @@
>         char *tmp, *tmp_str;
>  
>         len = *((SaUint32T *)buf);
> -       tmp = buf + sizeof (SaUint32T);
> +       tmp = buf + ALIGN_ADDR(sizeof (SaUint32T));
>  
>         if (len > 0) {
>                 tmp_str = amf_malloc (len + 1);
> @@ -1172,28 +1193,28 @@
>                 *str = NULL;
>         }
>  
> -       tmp += len;
> +       tmp += ALIGN_ADDR(len);
>  
> -       return tmp;
> +       return (tmp);
>  }
>  
>  char *amf_deserialize_SaUint32T (char *buf, SaUint32T *num)
>  {
>         *num = *((SaUint32T *)buf);
> -       return buf + sizeof (SaUint32T);
> +       return (buf + ALIGN_ADDR(sizeof (SaUint32T)));
>  }
>  
>  char *amf_deserialize_SaUint64T (char *buf, SaUint64T *num)
>  {
>         *num = *((SaUint64T *)buf);
> -       return buf + sizeof (SaUint64T);
> +       return (buf + ALIGN_ADDR(sizeof (SaUint64T)));
>  }
>  
>  char *amf_deserialize_opaque (char *buf, char *dst, int *cnt)
>  {
>         *cnt = *((SaUint32T *)buf);
> -       memcpy (dst, buf + sizeof (SaUint32T), *cnt);
> -       return buf + *cnt + sizeof (SaUint32T);
> +       memcpy (dst, buf + ALIGN_ADDR(sizeof (SaUint32T)), *cnt);
> +       return (buf + (ALIGN_ADDR(sizeof (SaUint32T)) + ALIGN_ADDR(*cnt)));
>  }
>  
>  void *_amf_malloc (size_t size, char *file, unsigned int line)
> 
> Index: amf.h
> ===================================================================
> --- amf.h       (revision 1241)
> +++ amf.h       (working copy)
> @@ -542,7 +542,8 @@
>         char *buf, int *size, int *offset, SaStringT str);
>  extern char *amf_serialize_SaUint32T (
>         char *buf, int *size, int *offset, SaUint32T num);
> -extern char *amf_serialize_SaUint64T (char *buf, SaUint64T num);
> +extern char *amf_serialize_SaUint64T (
> +        char *buf, int *size, int *offset, SaUint64T num);
>  extern char *amf_serialize_opaque (
>         char *buf, int *size, int *offset, char *cp, int cnt);
>  extern char *amf_deserialize_SaNameT (char *buf, SaNameT *name);
> 
> 
> 
> 2- On Solaris, the SA components executed have no names.
> 
> "ps -p <pid> -o comm" will show that the process has no command name.
> 
>      This is due to the way the execve() is called in openais-instantiate.c.
>      On linux, the command name of a component is shown between
>      square brackets, due to that same problem.
> 
> 
> Index: openais-instantiate.c
> ===================================================================
> --- openais-instantiate.c       (revision 1241)
> +++ openais-instantiate.c       (working copy)
> @@ -96,7 +96,7 @@
>                 /*
>                  * child process
>                  */
> -               res = execve (argv[1], &argv[2], envp);
> +               res = execve (argv[1], &argv[1], envp);
>                 if (res == -1) {
>                         return (errno);
>                 }
> 
> 
> 3- When killing the testamf1 component, it makes the aisexec process 
> crash on both of my nodes.
>      This comes from an "==" sign in the clc_cli_script file, instead of 
> an "-eg".
>      (Even with this fix, every times clc_cli_script returns a non-zero 
> value,
>      the aisexec process will crash. I've seen TODOs in that portion of 
> the code, so I stopped there).
> 
> Index: clc_cli_script
> ===================================================================
> --- clc_cli_script      (revision 1240)
> +++ clc_cli_script      (working copy)
> @@ -60,7 +60,7 @@
>  #      `cat $PIDFILEPATH/openais_cleanup_$SA_AMF_COMPONENT_NAME`
>         kill -9 `cat $PIDFILEPATH/openais_cleanup_$SA_AMF_COMPONENT_NAME`
>         STATUS=$?
> -       if [ $STATUS == 1 ]; then
> +       if [ $STATUS -eq 1 ]; then
>                 exit 0
>         else
>                 exit $STATUS
> 
> 
> 4- the testamf1 component also prints out a trace that it can't set the
>      scheduling class to RR with a priority of 99, because the max priority
>      for RR on solaris is 59.
> 
> Index: testamf1.c
> ===================================================================
> --- testamf1.c  (revision 1240)
> +++ testamf1.c  (working copy)
> @@ -299,11 +299,17 @@
>  
>  SaVersionT version = { 'B', 1, 1 };
>  
> -#if ! defined(TS_CLASS) && (defined(OPENAIS_BSD) || 
> defined(OPENAIS_LINUX) || defined(OPENAIS_SOLARIS))
> +#if ! defined(TS_CLASS)
> +#if (defined(OPENAIS_BSD) || defined(OPENAIS_LINUX))
>  static struct sched_param sched_param = {
>      sched_priority: 99
>  };
> +#elif defined(OPENAIS_SOLARIS)
> +static struct sched_param sched_param = {
> +    sched_priority: 59
> +    };
>  #endif
> +#endif
>  
>  void sigintr_handler (int signum) {
>         stop = FINALIZE;
> @@ -379,7 +385,7 @@
>  #if ! defined(TS_CLASS) && (defined(OPENAIS_BSD) || 
> defined(OPENAIS_LINUX) || defined(OPENAIS_SOLARIS))
>         result = sched_setscheduler (0, SCHED_RR, &sched_param);
>         if (result == -1) {
> -               fprintf (stderr, "%d: couldn't set sched priority\n", 
> (int)getpid());
> +               fprintf (stderr, "%d: couldn't set sched priority 
> (%s)\n", (int)getpid(), strerror(errno));
>         }
>  #endif
> 
> 
> Mathieu
> _______________________________________________
> Openais mailing list
> Openais at lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/openais
> 




More information about the Openais mailing list