> 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 :)
> This could have good implications for the other apis beyond coroipcc...
>
> Regards
> -steve
>
>>
>> _______________________________________________
>> Openais mailing list
>> [email protected]
>> https://lists.linux-foundation.org/mailman/listinfo/openais
>
_______________________________________________
Openais mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/openais
