[Openais] Re: Some event service code

Steven Dake sdake at mvista.com
Wed Jul 28 16:13:52 PDT 2004


Awesome.  Comments below...

On Wed, 2004-07-28 at 15:29, Mark Haverkamp wrote:
> Here is a patch for the event service.  The library side is pretty far
> along. The aisexec side has placeholders that just print something and
> return status.  I just wanted to run this by you for a review before I
> get too much farther into it.  I hope the coding style isn't too far
> off.  I've tried to reuse as much of my existing code as I could.  It's
> almost 2800 lines so rather than attach it, I put it here:
> 
> http://developer.osdl.org/markh/openais/openais_event_20040728.patch
> 
> Thanks,
> Mark.

Mark

Wow I must say I am impressed.  Excellent work !!

Thanks for the dependencies..  That was driving me crazy when I edited
the h files.

I do have a few comments of course :)

On the library:

I'd like the tabstops to look good at 4.  I think you are using tabstops
of 8.  This is a minor thing.  I think there is a way to tell vi to
force tabstops when it loads a file.  Do you know how this works?

we could probably put clust_time_now in ais_util.h/.c

In initialize, SaServiceConnect should free the queue allocated with
saQueueInit if there is an error.

In channel open, you don't do a handle put if handlecreate fails

in channel close, the first handle instance isn't put if the second
handle instance get fails.

All functions not specifically exported should be static: patt_size,
aispatt_to_evt_patt, filt_size, aisfilt_to_evt_filt

I know some of the code in clm, amf, checkpointing may not follow these
static guidelines, so if you see anything in any of the code, let me
know so I can fix it up.

It might help parallel style a little bit if the header names are
"header" vs ico_head, etc.  This definately isn't a big deal though.

You will have to change your executive handlers to take the struct
in_addr parameter in the latest tree.

One big problem to be avoided is duplicating services in several
places.  When I started openais some time ago, I had
amfComponentRegister, and there were two versions.  One for the library
which registered a component, and one for the executive.  This creates a
race condition.  If you send the message over the executive and handle
the request in one place "in your TODO: Add open code here" the race
conditions can be avoided  This can be applied all over the executive.

If you can fix up the library comments I'll merge that as one patch, or
if you want to wait for the entire thing we can.

I'm looking forward to see the remainder of the executive service
handler.

Regards
-steve




More information about the Openais mailing list