[Openais] Re: Some event service code

Mark Haverkamp markh at osdl.org
Thu Jul 29 08:15:28 PDT 2004


On Wed, 2004-07-28 at 16:13, Steven Dake wrote:
> 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?

I've always used tabstops of 8.  I didn't notice that your code was set
up for tabs at 4 since your code seems to look OK at 8. I don't know how
to set the tabstops at file load.  I think that there were some kernel
files that had some emacs comments to do that, I'm not sure about vi.

It looks like the only thing that looks funny is that the structure
declarations don't line up well at 4.    I'll run through the files and
line things up. 

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

Is that util.[ch] in the lib directory or new files?
 
> 
> 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.

Thanks, I knew that I probably missed a few things like that here and
there.

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

Good idea.  I normally try to do this.

> 
> 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.

The thing that I like about the prefix names is that when I use cscope
to browse the source code, I can easily distinguish different structure
elements.

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

OK.

> 
> 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.

Could you explain this again?  I'm not sure I understand what you are
saying here.

> 
> 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'll update the code and send out another patch for you to merge,
probably this afternoon.  Then I'll start filling in the server side
code and send updates as things get working.  If that sounds OK.

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

Thanks for the review.
Mark.


> Regards
> -steve
-- 
Mark Haverkamp <markh at osdl.org>




More information about the Openais mailing list