directory change notification patch

[Mark Weaver]

> Ok, a few comments. You're changing lib/select.c. This makes me very
> nervous. This is critical code, tested on millions of systems and took
> us a long time to get right. Are you sure your changes are correct ? Why
> do you need them ?
> 
I mentioned this in my first message -- basically it's as follows:

- smbd code assumes that (change notify) signals are delivered via EINTR 
when calling select.  Since notification polling is disabled in the 
present code if kernel change notification is active, the assumption 
would seem to be that the notification is reliable.  The mechanism for 
signal delivery is to write to a pipe in select.c (via 
sys_select_signal()) and report it later in sys_select.
- When a signal is delivered and any other fd is ready then sys_select 
does not return -1/EINTR, but yet reads from the pipe.  This leads to 
signal notifications being lost.  I chose to report EINTR in preference 
to any fds being ready (alternatively, you could not read from the pipe 
and report EINTR later -- this is simply a priority preference).  Note 
that I am assuming that the caller of sys_select is ready to deal with 
EINTR.
- sys_select_intr is a wrapper function for sys_select that eats EINTR. 
  This also looses signals.  I changed this function to simply call 
select() directly and copied other wrapper code from sys_select.  The 
rational was that there there is no point waiting on the signal pipe if 
the result is going to be tossed anyway.

The intention of the changes is to make signal delivery via sys_select 
reliable.  This appears to be fine in smbd, which calls sys_select in 
either oplock.c or process.c (when not running as a listener; the other 
call in server.c is not important).  The call in oplock.c is in 
receive_local_message; called via async_processing which deals with the 
change notify signals anyway.  The call in process.c just calls 
async_processing (hence deals with the notify signals).  The only other 
calls to sys_select in smbd are via sys_select_intr, which if signals 
are not eaten, is fine.

Thus, I think it is OK in smbd.  However, I agree that it may have 
unintended side effects.  I don't think that the change is wrong (if the 
code matches the intention at least :) -- but it is possible that other 
code that calls sys_select may be broken by the changes.  Wider 
review/testing would be in order.

In summary (and I probably should have said this at the start), the 
change is simply a fix to what appears to be the intended mechanism of 
signal delivery, which is currently broken.  You can test this quite 
easily for change notify by having a script change a directory 
repeatedly and observing that "kernel change notify on" is missing from 
the logs on occasion.  Since change notify is one-shot, this is somewhat 
of a disaster.

> Secondly, you're mixing direct POSIX calls (eg. open()) with Samba VFS
> calls. You can't do that. All calls touching the filesystem *must* go
> via the VFS calls.
> 
I can see one open call (change_notify_file_data_exists), which can 
trivially be changed (this is from the original patch).

I also have sys_getwd()/chdir().  I could not find a wrapper (e.g. 
sys_chdir()) for chdir() that would be a reversal of sys_getwd().  The 
intent is to ensure that the working directory is the same as when the 
change notification was posted, as the code that looks for changes 
relies on the this being the case.  Would SMB_VFS_GETWD/SMB_VFS_CHDIR be 
the more appropriate choice here?  My problem was that in the case that 
process_pending_change_notify_queue is called, I wished to save and 
restore the working directory.  Thus, for the first sys_getwd() call, I 
have no active connection to rely on.  Possibly that means is not 
necessary to save the working directory, but the function is called from 
a number of different locations, and I did not want to break existing code.

Finally, I have unlink.  This is used for deleting temporary TDB files 
when the "disk" method is used, and I could not find a wrapper for 
unlink.  These files all live in tmpdir(), and as such, have no 
relevance to the smbd client -- they should be a straight POSIX path.

> Just my first thoughts as I looked over it.
> 
I hope I've answered your questions.  Let me know wrt to the above and 
I'll try to make further improvements.

Thanks,

Mark