Re: [PATCH] use gnulib modules close (new) and open to hook into open/close

[Bruno Haible]

Hello Paolo,

Thank you for going ahead with this close() replacements merge! Faster
than I could do it.

Nice how you created the _gl_register_fd hook in the fchdir replacement.

> The Winsock replacement of close is triggered by the socket and accept
> modules.

I prefer and propose to not trigger the 'close' module from 'socket' or
'accept', and instead - just like for select - rely on the link error or
the warning of someone tries to use close() with sockets enabled
(recall: a link error when compiling on mingw, a warning when compiling
with GNULIB_POSIXCHECK on Linux).

While I agree that code that uses accept() or socket() is also likely to
use close(), this is only a heuristic, and it can err on both sides:
  - You can imagine code (in a library) that returns file descriptors to
    the caller. The library code may use socket() but not need close().
  - You can also imagine the opposite case, that needs close()
    but not socket() or accept() - programs that are invoked by parent
    programs that open stdin or stdout as sockets.
Therefore I find it better to stick with the overall gnulib approach:
the developer asks for certain modules and gets them and nothing more than
the necessary dependencies.

> whenever I need to force the replacement of
> open and close I use a macro gl_REPLACE_{OPEN,CLOSE} to avoid breaking
> encapsulation of m4/{open,close}.m4.

Yes, this is the right way to do it. And incidentally, by doing this, you
fixed a bug in m4/open.m4.

I have a couple of followup changes, to 19 files in total. Let me know what
you think of it. I'll then try to commit the thing in several understandable
steps, probably like this:
  1. fix preexisting bug in m4/open.m4.
  2. give a better structure to the fchdir replacement module.
  3. introduce the close module.
  4. update the tests dependencies.

The changes that I propose are in detail:

  - Declare all functions defined in one .c file and used in another .c file
    in a .h file. This is a basic principle, which
      1. avoids passing too few or too many arguments to a function after
         a couple of refactorings,
      2. avoids link errors if one of the .c files happens to be compiled
         in C mode and the other in C++ mode.

  - Change the return type of _gl_free_fd to 'void'.

  - Rename two internal functions
      _gl_free_fd -> _gl_unregister_fd, as it's the exact opposite of
                     _gl_register_fd,
      _gl_close_fd -> _gl_close_fd_maybe_socket, because I got confused between
                      rpl_close, _gl_close_fd, and _close.

  - In <sys/socket.h> add a warning if close() is used but <unistd.h> was not
    included.

  - In <unistd.h> provide a close() declaration with the same properties as
    for the functions in <sys/socket.h>: error if module 'close' not requested
    and compiling on mingw, warning if module 'close' not requested and
    compiling on Linux.

  - Keep an alphabetic order of the function declarations in <unistd.h>.

  - In close.c, don't duplicate the logic which is present in the <sys/socket.h>
    replacement. Rather, call _gl_close_fd_maybe_socket if and only if
    <sys/socket.h> declares it.

  - In winsock.c, no need any more to '#undef close'.

  - In winsock.c, define _gl_close_fd_maybe_socket only if the header file
    declares it. If gnulib module 'close' is not requested, there's no need
    to define _gl_close_fd_maybe_socket.

  - m4/close.m4: It's gl_REPLACE_CLOSE which assigns a value to REPLACE_CLOSE,
    therefore it's this macro which needs to require gl_UNISTD_H_DEFAULTS.

  - m4/fchdir.m4: There's no need to split off gl_FUNC_FCHDIR_BODY as a
    separate macro. And also no need for the gl_FUNC_CLOSE and gl_FUNC_OPEN
    expansions to occur before gl_FUNC_FCHDIR. The module dependency ensures
    that gl_FUNC_CLOSE and gl_FUNC_OPEN will be invoked; that's all we need.

  - m4/open.m4: Need to require gl_FCNTL_H_DEFAULTS before assigning
    REPLACE_OPEN.

  - m4/unistd_h.m4: Add a new variable UNISTD_H_HAVE_WINSOCK2_H. Semantically
    the same as HAVE_WINSOCK2_H, but I don't want to make the 'unistd' module
    depend on the 'sys_socket' module. Also, later, I'll need to trigger a
    similar declaration in <sys/ioctl.h>. There cannot be three modules which
    give a default value to HAVE_WINSOCK2_H (sys_socket, unistd, sys_ioctl).
    The workaround is to define three variables, each getting its default
    value from the particular module.

  - m4/sys_socket_h.m4: Update accordingly.

  - accept, socket: Remove the dependencies to the 'close' module.

  - close: Depend on 'unistd', because it provides the header file.

Ok?

Bruno

gnulib-close-addendum.patch
Description: Text Data