Re: [gamin] gam-poll.c rewrite

["Neal H. Walfield" <neal walfield org>]

>   Okay, good. But this breaks for me and this is a huge patch.
> The dnotify.py regression test breaks and nearly all subsequent
> ones.

I ran the tests suite and reported all the failures I saw in my email.
I realize now that I have not been configuring with
--enable-debug-api.  When I do this, I see the same failures as you.
See below for an explanation.

> Not breaking the regression tests on dnotify is really a hard condition
> to apply any patch at this stage unless we can understand and justify why:
> 
> [make tests output]
> bigfile.py
> noexists.py
> dnotify.py
> Traceback (most recent call last):
>   File "/u/veillard/gamin/python/gamin.py", line 107, in _internal_callback
>     self.callback (path, event, self.data)
>   File "./dnotify.py", line 26, in debug
>     if db_expect[dbg] != type:
> IndexError: list index out of range
> Traceback (most recent call last):
>   File "/u/veillard/gamin/python/gamin.py", line 107, in _internal_callback
>     self.callback (path, event, self.data)
>   File "./dnotify.py", line 26, in debug
>     if db_expect[dbg] != type:
> IndexError: list index out of range
> Traceback (most recent call last):
>   and then pages of errors.
> 
> Maybe it's just the debugging of kernel monitoring status which got
> broken, but it looks like your patches changes the behaviour of the kernel monitoring
> a lot, even in the simplest case. there are way too many dnotify event sent
> which mean way too many transition of state for the dnotify watches, and this
> occurs even on the simplest case, so in practice I think this is a serious
> breakage of the dnotify back-end.

Actually, the current implementation doesn't do enough monitoring.
Consider this example:

  #!/usr/bin/env python
  #
  import gamin
  import time
  import os
  import sys
  import shutil
  
  ok = 1
  top = 0
  expect = [gamin.GAMExists, gamin.GAMEndExist, gamin.GAMDeleted ]
  
  def callback(path, event, which):
      global top, expect, ok
      print "Got callback1: %s, %s" % (path, event)
      if event == gamin.GAMAcknowledge:
          return
      if expect[top] != event:
          print "Error got event %d expected %d" % (event, expect[top])
  	ok = 0
      top = top + 1
  
  shutil.rmtree ("temp_dir", True)
  os.mkdir ("temp_dir")
  mon = gamin.WatchMonitor()
  mon.watch_directory("temp_dir", callback, 0)
  mon.handle_events()
  os.rmdir ("temp_dir")
  time.sleep(1)
  mon.handle_events()
  mon.stop_watch("temp_dir")
  mon.handle_events()
  mon.disconnect()
  del mon
  
  if top != 3:
      print "Error: monitor got %d events insteads of 3" % (top)

The current code misses the deleted event because we don't monitor the
parent directory.  (You see the same behavioir with
s/watch_directory/watch-file/.)  So this is actually where most of the
extra events are coming from: we need to monitor the parent of a
directory for unlink events.  Currently, the code watchs every
ancestor even though you only need to watch the parent if the
directory in question is empty.  However, that is non-trivial and may
involve some races (if the last file is unlinked and then the
directory as well we might miss the chance to set up the monitor for
the parent) so I deferred.

>   So as is that patch which is nearly 100KB doesn't pass. Could you chunk it
> into digestible pieces with a well identified purpose ? It will allow me to
> review them and help pinpoint where the problem is being introduced.

This patch really makes a single major change: it tries to handle the
MON_ flags more intelligently.  These changes are deeply penetrating
and intimately linked with the kernel back end and how MON_WRONG_TYPE
functions.  I admit that the patch does include some refactoring of
code, documentation improvements as well as a few algorithmic shifts
e.g. from O(N) algorithms to O(1) and the consolidation of
gam_exclude_check.  Hence, there is some low hanging fruit here and
with a bit of work, I think I could make the patch about 30% smaller.
Honestly, I think it will be difficult to break up the bulk of the
patch and if I did manage to separate say the MON_WRONG_TYPE
functionality from the rest, there would still be a strong dependency
between the two (in both directions).

I understand that the patch is large but the changes are by their
nature deep. I have a number of changes which depend on this patch
which I haven't included so as to make this patch as small as
possible.  Specifically, polling files and directories if the kernel
back end is unable to monitor a node, e.g. if there are no more file
descriptors available or a permission check fails; introducing back
end capabilities so that back ends can use only what they actually
need from the polling code, e.g. GAM_WATCH_LINK and GAM_WATCH_UNLINK
to indicate that the back end can see directory link and unlink events
and thus the poll back end needn't worry about this and
GAM_WATCH_CONTENTS if the back end can see changes in files in the
directory.

Where do you want to go from here?

Thanks,
Neal