Re: [RFC PATCH] fchdir: move fd shadowing into fd-hook

[Eric Blake]

On 04/28/2011 05:20 AM, Bruno Haible wrote:
> Hi Eric,
> 
>> In order to eventually add socket nonblocking status into
>> fd shadowing, it is useful to move the existing fchdir name
>> shadowing into a common file.
> 
> No, that's definitely not useful. On the contrary, it would make it
> harder to write and maintain the code that stores the socket non-blocking
> status in an array.

My vision was to have a single shadow array, maintained by fd-hook,
where each element of the shadow array can contain multiple elements
according to what other modules are in use:

If fchdir is in use, the shadow array contains char* names of directory fds
If dirfd is in use, the shadow array contains DIR* counterparts to
directory fds
If nonblocking is in use, the shadow array contains bool nonblocking
state of socket fds

Then, all APIs that can manipulate fds must call a single hook into
array manipulation (dup, dup2, dup3, fcntl, open, [fopen], [freopen],
close, socket, [socketpair], accept, accept4).

But it sounds like you would rather maintain two or three shadow arrays
- one array in fchdir for dir names, one array in nonblocking for
nonblocking status.  Am I understanding you correctly?

> 
> Gnulib is very modular, and therefore requires techniques to keep
> independent code pieces in independent files, more than average programs.
> 
> We currently have two "hooks" for file descriptor operations:
>   - fchdir.c, that stores the name of directories.
>   - fd-hook.c and sockets.c, for close() and ioctl() on sockets.
> We want to add a third one:
>   - that stores the non-blocking status of file descriptors.
> 
> You're saying: let's merge the first two, so that adding the third one will
> be easier. But this kills the modularity: separate things get mixed.
> Instead, we need abstract infrastructure so that the third one can be added
> _without_ affecting the first two ones.
> 
> It's really different things: In fchdir.c we deal only with file descriptors
> that are directories, and store a string per fd. For the non-blocking status
> we will deal only with sockets, and store a boolean per fd. The only common
> thing is that both will need to hook into the 'dup', 'dup2', 'dup3', 'fcntl',
> 'close' functions.
> 
> You know what a CLOS :AROUND method and :AFTER method is? In CLOS [1][2][3],
> you can hook in additional code to every function. This extra code is called
> a "method". There are :AFTER methods, which are run after the function's
> original code. In Java this concept is known as "listeners"; in Emacs Lisp 
> it's
> called a "hook". There are also :AROUND methods, which shadow that function's
> original code but have the option to invoke that code.

So you're saying that the better approach would be to expand fd-hook to
allow registration of various :AROUND methods, and that any combination
of the fchdir, socket, and/or nonblocking modules registers up to three
separate :AROUND method for 'close'.

> 
> We are doing this kind of things constantly in gnulib, but have it hardwired
> in most places (for example lib/lstat.c is a typical code for an :AROUND 
> method
> added to lstat()). In the second case, however, we need the abstract
> infrastructure, because we would get link dependencies otherwise.
> 
> So, in the CLOS speak,
>   - fchdir.c are :AROUND methods for 'dup', 'dup2', 'dup3', 'fcntl', 'close',
>     open(), fdopendir().
>   - fd-hook.c and sockets.c are :AROUND methods for 'ioctl', 'close'.
>   - for the non-blocking status we need :AROUND methods for 'dup', 'dup2',
>     'dup3', 'fcntl', 'close'.

nonblocking also needs :AROUND for 'socket', 'socketpair', 'accept',
'accept4'

> 
> Now there are two ways to proceed:
> 
>   a) Formalize the notion of a set of :AROUND methods for 'dup', 'dup2',
>      'dup3', 'fcntl', 'close', in such a way that the first and third code
>      can share the same infrastructure. That would mean a file fd-hook2.c,
>      similar to fd-hook.c but just with other functions to hook in.
> 
>   b) Search for all uses of "#if REPLACE_FCHDIR" and add a
>      "#if WINDOWS_NONBLOCKING" section next to it (before or after, does not
>      matter, since they operate independently).
> 
> Approach a) is IMO overkill, because we don't have to deal with link
> dependencies for the third case, like we didn't need for fchdir.
> 
> So I think approach b) is the way to go.

But that goes back to my question - are you proposing that fchdir and
nonblocking maintain separate shadow arrays?  And what about my idea of
starting to associate DIR* with directory fds, for the sake of dirfd?

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature