[Openais] proposing patches for AMF

Mathieu Marie Mathieu.Marie at Sun.COM
Fri Sep 22 06:51:17 PDT 2006


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



More information about the Openais mailing list