Re: event engine patch update

[Jan-Henrik Haukeland]

Martin Pala <address@hidden> writes:

> Yes :)
>
> In the attachment is version for current cvs sources. In addition to
> last mentioned patch it fixes compile time warning for deprecated
> usage of casted expression as lvalue in net.c (not related to event
> engine refactoring).
>
> I don't know any refactoring related bug currently - it seems that it
> works well (but it still needs further testing). I think the patch is
> ready for checkout.

Well, that was a big patch! I have just browsed through it and
concentrated the browsing around the datastructure changes in
monitor.h and read event.c/h and validate.c. The first impression is
that this looks like an improvement and refactor away the weakness in
the orginal implementation.

As far as I can see, you refactor out the internal even handling in
validate.c and centralize it in event.c, this is good. Event handling
in validate.c is basically reduced to post an event for any and all
tests in validate, which is also good since you now have a one single
unified interface into the event machinery from validate, i.e.
Event_post() and not the ugly internal flag settings we had before.

Events posted via Event_post() are put on an event list and handled
based on certain conditions in event.c:handle_event().

Did I get it right? 

I think this patch is an improvement and I'm +1 for checking this into
CVS. But first I have a few comments and questions

1) Have you run monit with this patch through valgrind? To me it looks
like events are added to the list but not dequeued and that there is a
massive memory leak here. Event_free() is not called at all in the code.

2) In validate.c, the report parameter has bugging me for a while. Can
you please get rid of it by calling Event_post() in the lower
function? :-) E.g. check_uid() and check_gid() can just call
Event_post() directly. This makes the code simpler as well.

3) Please use the "one-true bracket style" in the code i.e. 

         if(x) {
             blabla
         }

   and also break lines at 79 column width. Apropos I found out that
   it is possible to use 'C-c .' to set the C indentation style (to
   gnu) in (X)emacs, which is the style we use.

4) It would be nice (but not required) if you could give a short
"formalized" description of the new event machinery and data
structure. For example a drawing and/or text. After all this is a
pretty important part of the code and it would be good to have an
overview :)

Good work Martin!!

-- 
Jan-Henrik Haukeland