[gamin] Re: gam_node.c cleanup

At Mon, 13 Jun 2005 08:09:08 -0400,
Daniel Veillard wrote:
>   However I'm a bit uneasy with transforming casual return handling in case
> of NULL parameters, sometimes it's just cumbersome to test NULL

True.  My own philosophy is to be conservative in this regard and only
permit dubious parameters when actually necessary.  That is, allow the
caller program to pass NULL once a situation has been demonstrated
where it is useful; not assume that a situation will manifest itself.

>   Applying your patch (please use mail attachment rather than including in
> the mail) for example makes some of the regression tests just crash:
>     make[1]: Entering directory `/u/veillard/gamin/tests'
>     [...]
>     running test 8

Okay, I found it.  It appears the problem is in the gamin poll code.
I missed it because I was testing on my port and we don't use the
polling backend in the Hurd notify code.  Apparently, dnotify does
when a file/directory does not exist.  The attached patch fixes the
regression and make check passes.  (Although I can't get the python
test suite to run here for some reason...)

The only caller of gam_poll_first_scan_dir is
gam_poll_consume_subscriptions.  In the case where the directory does
not exist, _first_scan_dir would lookup the node corresponding to
dpath (creating it if it did not exist) and add the subscription to
it.  This was unnecessary: dir_node is the node cooresponding to dpath
and sub was added to dir_node immediately before
_consume_subscriptions called _first_scan_dir.

> I.e. adding assert without checking all calling points first for the
> test trades immediate instability over long term code maintainance.
> All code in server/ is internal code. It's not API code, so IMHO
> stability is more important than API/code maintainance.

I can appreciate this stance for stable code, however, in development
code it seems a bit unusual.

>   Oh and documetnation updates are very very welcome ! If you revamp
> some of it please use the


> /**
>  * functionname:

Just to be clean: you want to repeat the name of the function?

>  * @arg1: ...
>  * @argn: ...
>  *
>  * Description of the function
>  *
>  * Returns description of the return value if non void

Shouldn't this be @returns ...

>   I don't know what do to with your current patch, there are good points
> but I don't feel like commiting if it breaks make tests. Until either of us
> get to fix the breakage, I don't think the asserts should be
> commited.

Okay now?


Attachment: gam-poll.diff
Description: Binary data

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]