Re: gnulib broken on systems lacking fchdir

[Eric Blake]

Matthew Woehlke <mw_triad <at> users.sourceforge.net> writes:

> > A well-written fchdir module would be welcome.  Such a module would have
> > no effect on coreutils proper, other gnulib modules, or any system with
> > fchdir support.
> 
> Ok, thanks... Anyway, here's a start:
> 
> struct fd_heap_node {
>    int fd;
>    const char * path;
> }

Depending on your choice of representation, it may be more efficient to go 
directly with an array of char*, rather than a list of nodes, since open() 
always picks the lowest currently unused fd (ie. an array is not likely to be 
very sparse, so use the array index implicitly rather than duplicating it in 
your data structure).

> 
> int
> fchdir (int fd)
> {
>    struct fd_heap_node n = fdheap_get(fd);
>    if (!n)
>      {
>        errno = EBADF;
>        return -1;
>      }
>    return chdir (n->path);
> }
> 
> int
> open (const char* pathname, int flags)

You need to remember the optional mode argument to open.

> {
>    int fd = SYS_open (pathname);
>    if (fd >= 0)
>      fdheap_add (fd, pathname);

Make sure fdheap_add duplicates the name; if you go with xstrdup, then your 
replacement should be GPL (since LGPL implies library code, but libraries 
shouldn't arbitrarily die on memory allocation failures).  Also, GNU coding 
standards recommend using just 'name', rather than 'filename' or even 'file'.

> 
>    return fd;
> }
> 
> int
> close (int fd)
> {
>    int res = SYS_close (fd);
>    if (res == 0)
>      fdheap_del (fd);
> 
>    return res;
> }
> 
> ...but as I said, I'm not really familiar with how one needs to overload 
> open()/close(), or if fopen()/fclose() and who-knows-what-else also need 
> to be overridden. 

Actually, gnulib already has an example of overloading open, via the *-safer 
modules.  The idea is that you arrange for <config.h> to #define open rpl_open, 
then all your code that directly calls open (except for the one .c file where 
you actually implement rpl_open) will get your replacement.  But you are 
correct that existing library code that already calls open under the hood (such 
as fopen) will not go through your wrapper, so you have to decide in advance 
how many functions need to be wrapped.

I think a first thing to do would be to see where fchdir is used in the fts 
implementation.  If we can guarantee that fts always uses fchdir on fds that it 
obtained by open and/or opendir/dirfd from within fts, rather than on random 
fds from external sources, then that is all the more you would have to wrap 
(calling fchdir(fileno(fopen(...))) would then need to fail; and I would try to 
make the replacement fchdir smart enough to distinguish EBADF for non-open fd 
vs. EOPNOTSUPP for open fd not found in the fd-to-name map).  In other words, 
you don't have to replace ALL uses of open, only the uses that are likely to be 
a target of fchdir.

> 
> What about the FD table; should it be a hash table, a binary tree, an 
> ordered linked list, or something else entirely?

Gnulib already provides the gl_list module.  The idea there is that you start 
by coding with an array list (probably a good choice anyways, since the 
underlying kernel also maintains an array of open fds, and since it seems to me 
that you are always going from fd to name, never a reverse lookup), work out 
the bugs, then decide if some other representation, such as an AVL tree list, 
would be more efficient for the typical usage pattern of the list.  Once you 
use the gl_list API, it is only a one-line code change and reinvocation of 
gnulib-tool to pick up the new underlying list implementation.

> 
> (Can autotools make compilation of this dependent on HAVE_FCHDIR so that 
> it doesn't need to be in One Big Preprocessor Condition? Am I guessing 
> right that this - with overloading open()/close() correctly - would mean 
> that no header is needed?)

Yes.  With m4 files, it is possible to make configure conditionally compile in 
the fchdir replacement and all its needed friends only on platforms where 
fchdir is missing, as long as each group of related replacement functions live 
in their own .c file.  And if you properly use AC_CACHE, you can even test your 
replacement code on a system that DOES have fchdir by setting the cache 
variable to fake out the ./configure check to force your code to be enabled.  
In short, all the magic for deciding and providing the replacement is in 
the .m4 file at configure time; every other module that uses fchdir has to pick 
up a new "fchdir.h" header, but can otherwise assume POSIX semantics with no 
further changes.

-- 
Eric Blake