[Openais] Re: just looked at some of the openais code, have some comments

[Steven Dake]

On Tue, 2004-06-29 at 10:24, Chris Friesen wrote:
> Steven Dake wrote:
> > I am likely to make a
> > portability library of things like threads/condition variables/reference
> > counting for the library.  That library could conditionally noop the
> > code for threads if necessary.
> 
> Sounds good.
> 
> > I feel a little uncomfortable, as a library, changing signal handlers.
> > The SA Forum AIS WG is in process of specifying what exactly happens in
> > a signal handler (should APIs be allowed or not).  I'll take this input
> > to the specs group.
> 
> Yeah.  It's always a bit of a dilemma as to how to deal with signals.  You could 
> just put a stake in the ground and say that no library functions are async-safe, 
> so if the user wants to use them in signal handlers they would just have to 
> block signals around all library calls.
> 
> > You mean add a flag to saClmDispatch?  The downside of that approach is
> > that the API then becomes nonstandard for a small performance payoff.
> 
> Yes, that's what I had in mind. However, I had forgotten that there were 
> standards that you were coding to.
> 
> > I'd like to match the AIS standard as much as possible, even when it
> > means performance won't be right.  The longer-term approach to this
> > problem is to drive that kind of feedback into the specs group which
> > I'll do.
> 
> Cool.  That'd be great.
> 
> > I see the light now...  Refcounting is the way to go, especially for
> > handling dispatch and finalize calls.  Then the mutex wont have to be
> > held across callbacks and a bunch of other problems go away too.
> 
> I'm glad you like the refcount idea.  I don't know how familiar you are with the 
> kernel code, but they use refcounts all over the place.
> 
> I've been looking at the code a bit more and have a few other comments:
> 
> 1) In the test programs, you've got select() being called in while loops.  When 
> select() returns, the fd sets will be modified to reflect what fds had events 
> occur.  If you get a signal, you can exit select() with empty fd sets, and you 
> will no longer monitor anything.  You need to either copy the fd set to a 
> working one each loop, or else recreate the fd set each time through the loop.
> 

Yup your right.  The test programs are hacky.  Maybe I'm dreaming, but
I'd like to see the openais project create a thourough test suite that
provides 100% code coverage for the openais codebase.

> 2) There's a small niggle in testamf.c, HealthcheckCallback().  The print 
> statement will run every time since the loop is commented out, but it prints 
> that 20 checks have occurred.
> 

Yup I'll fix this

> 3) It might be nice to be able to set the cluster name and node name.
> 

Yes cluster name is on the TODO list.  I'll add node name to the TODO. 
What kind of name were you thinking?  uname -n?

> 4) The magical version of
> 
>   SaVersionT version = { 'A', 1, 1 };
> 
> is a bit confusing.  I wonder if it could be made a constant or something rather 
> than having to be explicitly typed out by the user?
> 

This version is required by the specifications, and your right, it is
confusing.  I want to avoid implementing non-spec compliant code if at
all possible, but one option is to add a "AISVersionA0101" structure to
the library that anyone can use in function calls.

Thanks
-steve

> Chris
> 
>