[Openais] [Corosync] [Patch] [FreeBSD] coroipcc_service_connect() returns 0 in some cases
Steven Dake
sdake at redhat.com
Tue Mar 30 09:50:08 PDT 2010
On Tue, 2010-03-30 at 10:40 +0200, Jerome Flesch wrote:
> Steven Dake a écrit :
> > On Tue, 2010-03-30 at 09:57 +0200, Jerome Flesch wrote:
> >>> Patch looks good for commit
> >>>
> >> Committed.
> >>
> >>
> >>> responses to questions inline
> >>>
> >>> On Mon, 2010-03-29 at 11:10 +0200, Jerome Flesch wrote:
> >>>> Hello,
> >>>>
> >>>> I just had a weird case where SaClmInitialize() returned the value 0 to
> >>>> me. Since this value isn't valid for a enum SaAisErrorT, I investigated
> >>>> and I found out that corosync/coroipcc.c:coroipcc_service_connect() can
> >>>> return 0 in case Corosync was compiled with _POSIX_THREAD_PROCESS_SHARED
> >>>> <= 0 (ie on a FreeBSD system) when there is a problem with the IPC
> >>>> semaphores. This case is actually likely to happen on a FreeBSD system
> >>>> due to IPC default limits.
> >>>>
> >>>> In the process, I also changed the way the variable 'res' was used:
> >>>> Since it's an enum, I've assumed it doesn't really make sense to use it
> >>>> to store return values of system calls, so I replaced it by sys_res.
> >>>>
> >>>> Also, I was wondering if there could be a way to get more details when
> >>>> coroipcc_service_connect() fails. CS_ERR_LIBRARY and CS_ERR_TRY_AGAIN
> >>>> are not exactly what I would call meaningful error codes ... It's not
> >>>> very handy to have to try to guess if it failed because permissions on
> >>>> the Unix socket are wrong or because IPC limits have been reached or
> >>>> because an upper layer failed for some reason. I assume adding error
> >>>> codes like CS_ERR_SEM, CS_ERR_SHM, could be an option but letting such
> >>>> level of implementation details be visible in the error codes is not a
> >>>> valid option, right ?
> >>>>
> >>> yes i prefer to have less error codes with no implementation details.
> >>>
> >>>> Right now, when I have to diagnose this kind of issue, I'm running a
> >>>> patched version of Flatiron including the following macro (in lib/util.h):
> >>>>
> >>>> +#ifdef DEBUG
> >>>> +#define DPRINTX(...) do { \
> >>>> + fprintf(stderr, "Corosync lib: %s:L%d: ", __FILE__, __LINE__); \
> >>>> + fprintf(stderr, __VA_ARGS__); \
> >>>> + fprintf(stderr, "\n"); \
> >>>> + } while (0)
> >>>> +#else
> >>>> +#define DPRINTX(...)
> >>>> +#endif
> >>>>
> >>>> Is there a more adequate way of getting debug information from the
> >>>> libraries themselves ?
> >>>>
> >>>> I was thinking of a slightly more elegant way of doing it. I could add
> >>>> functions like for example: 'void coroipcc_set_lasterror(const char
> >>>> *str)', 'const char *coroipcc_get_lasterror(void)' ? Using
> >>>> pthread_setspecific(), it would be possible to make it thread-safe. Do
> >>>> you think it would be worth I spend some time on making a patch for that ?
> >>>>
> >>> sounds interesting. I wouldn't mind some diagnostic capability coming
> >>> out of the apis, but the thread safety was a problem I didn't know how
> >>> to solve. After reading the man page for pthread_setspecific, it looks
> >>> like it might solve the problem.
> >>>
> >>> I would prefer apis something like this in coroipcc.c
> >>> static void coroipcc_lasterror_set (const char *fmt, ...);
> >>>
> >> It makes sense. It would allow to get more details like for instance
> >> strerror(errno).
> >>
> >>
> >>> and in coroipcc.h
> >>> extern const char *coroipcc_lasterror_get (void).
> >>>
> >>> Curious how you plan to create the key and garbage collect the
> >>> pthread_key values...
> >>>
> >> It's pretty simple actually: pthread_key_create() allows to attach a
> >> destructor to a pthread key. This destructor is called each time a
> >> thread stops, with as argument the pointer stored with
> >> pthread_setspecific for this thread. So if we store a simple buffer with
> >> pthread_setspecific, we can simply attach 'free()' to the key :)
> >>
> >>
> >
> > How do you pthread_key_create? Use pthread_once in each api call? I
> > want to avoid a global init function for apis. pthread_once may have
> > performance implications if it takes a mutex each time it runs.
> >
> Hm, good point. However, coroipcc_lasterror_set() /
> coroipcc_lasterror_get() would only be called in case of error(s), which
> shouldn't happen too often (except maybe for CS_ERR_TRY_AGAIN ?), so
> having to lock a mutex in theses cases wouldn't be too expensive, no ?
>
So only call pthread_once() on lasterror_set()? I am satisified if
these error reporting systems are a little slow. I don't want to impact
the normal data paths however.
>
> > Another option is to use a constructor section in the library.
> >
> You mean defining a function like "static void coroipcc_init(void)
> __attribute__ ((constructor))" ? Wouldn't that make portability issues ?
>
>
I suppose, but at the moment, corosync requires gcc to run properly.
the constructor functionality is used all over the code.
> > Like to see a patch..
> >
> > Regards
> > -steve
> >
> >>> This could have good implications for the other apis beyond coroipcc...
> >>>
> >>> Regards
> >>> -steve
> >>>
> >>>> _______________________________________________
> >>>> Openais mailing list
> >>>> Openais at lists.linux-foundation.org
> >>>> https://lists.linux-foundation.org/mailman/listinfo/openais
> >
>
More information about the Openais
mailing list