Re: [gamin] release soon?

[John McCutchan <ttb tentacle dhs org>]

On Mon, 2005-08-01 at 14:58 -0400, Daniel Veillard wrote:
> On Mon, Aug 01, 2005 at 12:04:20PM -0400, Daniel Veillard wrote:
> > On Mon, Aug 01, 2005 at 11:29:27AM -0400, John McCutchan wrote:
> > > Yo Daniel,
> > > 
> > > I have committed a bunch of my changes to CVS. I have a large inotify
> > > backend rewrite that I don't want to commit until we make a release. I
> > > need to do some testing of the current tree, but maybe you could make a
> > > release tomorrow?
> > 
> > Yeah, I was thinking about it. Maybe later today, or tomorrow, yes definitely!
> 
>   You made 2 huge commits in CVS head, now a lot of regression tests just
> fail on my stable dnotify machine (inotify is still not part of an official
> kernel realease it really has to work), I really can't release in that shape.
> Please refrain from accumulating such large changes and commiting them all in
> a block, and always make sure the make tests don't show any error (or justify
> the change so it still passes) before any commit.
>   Now what should I do revert or wait ? I would say revert because I
> was thinking of a release and the release stuff 1/ should have make tests
> pass and 2/ CVS users should have had some time to run it prior to the release.


http://cvs.gnome.org/bonsai/cvsquery.cgi?branch=&dir=gamin&who=jmc&date=explicit&mindate=2005-08-01%2011:08&maxdate=2005-08-01%2011:10

Looking at that commit. There are no behaviour changes at all.

http://cvs.gnome.org/bonsai/cvsquery.cgi?branch=&dir=gamin&who=jmc&date=explicit&mindate=2005-08-01%2011:26&maxdate=2005-08-01%2011:28

Now, there are a few behaviour changes in that commit. I'm going to
paste them below so we can discuss.

gam_node.c:
jmc:  @@ -189,9 +191,7 @@
jmc:   gam_node_remove_subscription(GamNode * node, GamSubscription * sub)
jmc:   {
jmc:       g_assert(node);
jmc:  -
jmc:  -    if (!g_list_find (node->subs, sub))
jmc:  -	    return TRUE;
jmc:  +    g_assert(g_list_find (node->subs, sub));

I think this is correct. We should never be removing a non-existant
subscription from a node.

gam_poll.c:
jmc:  @@ -149,6 +149,9 @@
jmc:   static void
jmc:   trigger_dir_handler(const char *path, pollHandlerMode mode, GamNode * node)
jmc:   {
jmc:  +    if (node->mon_type != GFS_MT_KERNEL)
jmc:  +	    return;
jmc:  +

So, the dir handler won't be called unless the node monitor type is KERNEL.
By default I have included these settings:

ext3 KERNEL
ext2 KERNEL
nfs POLL
novfs POLL
everything else KERNEL

Are you running the tests in an NFS filesystem? Otherwise I can't see
this causing a change.

gam_poll.c:
jmc:  @@ -452,11 +458,16 @@
jmc:       int stat_ret;
jmc:       const char *path;
jmc:   
jmc:  +    /* If not enough time has passed since the last time we polled this node, stop here */
jmc:  +    if (current_time - node->lasttime < node->poll_time) 
jmc:  +	    return 0;
jmc:  +


Now, here is a change I could actually see causing regressions. I have included poll_time
which controls how often a directory can be polled. By default I set this to 1 for local file systems.

This is probably screwing up flow control. 

gam_poll.c:
jmc:  @@ -1060,8 +1074,9 @@
jmc:        * make sure the subscription still isn't in the new subscription queue
jmc:        */
jmc:       if (g_list_find(new_subs, sub)) {
jmc:  -        GAM_DEBUG(DEBUG_INFO, "new subscriptions is removed\n");
jmc:  +        GAM_DEBUG(DEBUG_INFO, "new subscription is removed\n");
jmc:           new_subs = g_list_remove_all(new_subs, sub);
jmc:  +	return TRUE;
jmc:       }

There is that missing return. Without the return, kernel ref counts will go down here. Eventually this will cause a crash.

jmc:  @@ -150,11 +152,13 @@
jmc:   gboolean
jmc:   gam_add_subscription(GamSubscription * sub)
jmc:   {
jmc:  -    
jmc:       if (sub == NULL)
jmc:           return(FALSE);
jmc:   
jmc:  -    return (gam_backend_add_subscription(sub));
jmc:  +    if (gam_fs_get_mon_type (gam_subscription_get_path (sub)) == GFS_MT_KERNEL) 
jmc:  +	    return (gam_backend_add_subscription(sub));
jmc:  +    else
jmc:  +	    return gam_poll_add_subscription (sub);
jmc:   }

Again, just a check if we should be using kernel monitoring or not. Unless you are running on an NFS partition, this shouldn't change any thing.


So the most likely change to be causing a problem is:
gam_poll.c:
jmc:  @@ -452,11 +458,16 @@
jmc:       int stat_ret;
jmc:       const char *path;
jmc:   
jmc:  +    /* If not enough time has passed since the last time we polled this node, stop here */
jmc:  +    if (current_time - node->lasttime < node->poll_time) 
jmc:  +	    return 0;
jmc:  +

I'd be fine with this code being reverted for this release. The rest of my code changes are mainly cosmetic, or are pure bug fixes.

-- 
John McCutchan <ttb tentacle dhs org>