[Openais] [Corosync] [Patch] [FreeBSD] coroipcc_service_connect() returns 0 in some cases
Jerome Flesch
jerome.flesch at netasq.com
Tue Mar 30 01:40:04 PDT 2010
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 ?
> 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 ?
> 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