[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