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
--- lib/fcntl.in.h.orig 2008-10-09 02:51:09.000000000 +0200
+++ lib/fcntl.in.h 2008-10-09 01:21:51.000000000 +0200
@@ -56,6 +56,11 @@
# endif
#endif
+#ifdef FCHDIR_REPLACEMENT
+/* gnulib internal function. */
+extern void _gl_register_fd (int fd, const char *filename);
+#endif
+
#ifdef __cplusplus
}
#endif
--- lib/sys_socket.in.h.orig 2008-10-09 02:51:10.000000000 +0200
+++ lib/sys_socket.in.h 2008-10-09 02:30:18.000000000 +0200
@@ -138,6 +138,11 @@
/* Wrap everything else to use libc file descriptors for sockets. */
+# if @HAVE_WINSOCK2_H@ && !defined _GL_UNISTD_H
+# undef close
+# define close close_used_without_including_unistd_h
+# endif
+
# if @GNULIB_SOCKET@
# if @HAVE_WINSOCK2_H@
# undef socket
@@ -370,6 +375,12 @@
# define select select_used_without_including_sys_select_h
# endif
+# if @GNULIB_CLOSE@ && @HAVE_WINSOCK2_H@
+/* gnulib internal function. */
+# define HAVE__GL_CLOSE_FD_MAYBE_SOCKET 1
+extern int _gl_close_fd_maybe_socket (int fd);
+# endif
+
# ifdef __cplusplus
}
# endif
--- lib/unistd.in.h.orig 2008-10-09 02:51:10.000000000 +0200
+++ lib/unistd.in.h 2008-10-09 01:59:09.000000000 +0200
@@ -75,6 +75,25 @@
#endif
+#if @GNULIB_CLOSE@
+# if @REPLACE_CLOSE@
+/* Automatically included by modules that need a replacement for close. */
+# undef close
+# define close rpl_close
+extern int close (int);
+# endif
+#elif @UNISTD_H_HAVE_WINSOCK2_H@
+# undef close
+# define close close_used_without_requesting_gnulib_module_close
+#elif defined GNULIB_POSIXCHECK
+# undef close
+# define close(f) \
+ (GL_LINK_WARNING ("close does not portably work on sockets - " \
+ "use gnulib module close for portability"), \
+ close (f))
+#endif
+
+
#if @GNULIB_DUP2@
# if [EMAIL PROTECTED]@
/* Copy the file descriptor OLDFD into file descriptor NEWFD. Do nothing if
@@ -112,14 +131,6 @@
environ)
#endif
-#if @GNULIB_CLOSE@
-# if @REPLACE_CLOSE@
-/* Automatically included by modules that need a replacement for close. */
-
-# define close rpl_close
-extern int close (int);
-# endif
-#endif
#if @GNULIB_FCHDIR@
# if @REPLACE_FCHDIR@
@@ -385,6 +396,12 @@
#endif
+#ifdef FCHDIR_REPLACEMENT
+/* gnulib internal function. */
+extern void _gl_unregister_fd (int fd);
+#endif
+
+
#ifdef __cplusplus
}
#endif
--- lib/close.c.orig 2008-10-09 02:51:09.000000000 +0200
+++ lib/close.c 2008-10-09 02:30:14.000000000 +0200
@@ -15,33 +15,31 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include <config.h>
+
+/* Specification. */
#include <unistd.h>
-#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
-# if defined GNULIB_SOCKET || defined GNULIB_ACCEPT
-# define GNULIB_NEED_CLOSE_FD 1
-# endif
+#if GNULIB_SYS_SOCKET
+# include <sys/socket.h>
#endif
+
/* Override close() to call into other gnulib modules. */
int
rpl_close (int fd)
#undef close
{
-#ifdef GNULIB_NEED_CLOSE_FD
- extern int _gl_close_fd (int fd);
- int retval = _gl_close_fd (fd);
+#if HAVE__GL_CLOSE_FD_MAYBE_SOCKET
+ int retval = _gl_close_fd_maybe_socket (fd);
#else
int retval = close (fd);
#endif
#ifdef FCHDIR_REPLACEMENT
if (retval >= 0)
- {
- extern int _gl_free_fd (int fd);
- _gl_free_fd (fd);
- }
+ _gl_unregister_fd (fd);
#endif
+
return retval;
}
--- lib/fchdir.c.orig 2008-10-09 02:51:09.000000000 +0200
+++ lib/fchdir.c 2008-10-09 01:24:13.000000000 +0200
@@ -1,5 +1,5 @@
/* fchdir replacement.
- Copyright (C) 2006, 2007 Free Software Foundation, Inc.
+ Copyright (C) 2006-2008 Free Software Foundation, Inc.
This program is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -78,13 +78,8 @@
/* Hook into the gnulib replacements for open() and close() to keep track
of the open file descriptors. */
-/* We do not want to declare these anywhere, but we want prototypes to
- shut up -Wimplicit. */
-extern int _gl_free_fd (int fd);
-extern void _gl_register_fd (int fd, const char *filename);
-
-int
-_gl_free_fd (int fd)
+void
+_gl_unregister_fd (int fd)
{
if (fd >= 0 && fd < dirs_allocated)
{
@@ -121,7 +116,7 @@
int retval = closedir (dp);
if (retval >= 0 && fd >= 0 && fd < dirs_allocated)
- _gl_free_fd (fd);
+ _gl_unregister_fd (fd);
return retval;
}
--- lib/open.c.orig 2008-10-09 02:51:09.000000000 +0200
+++ lib/open.c 2008-10-09 01:13:58.000000000 +0200
@@ -33,14 +33,14 @@
/* Specification. */
#include <fcntl.h>
-# include <errno.h>
-# include <stdarg.h>
-# include <string.h>
-# include <sys/types.h>
-# include <sys/stat.h>
+#include <errno.h>
+#include <stdarg.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
int
-rpl_open (const char *filename, int flags, ...)
+open (const char *filename, int flags, ...)
{
mode_t mode;
int fd;
@@ -61,12 +61,12 @@
va_end (arg);
}
-# if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
if (strcmp (filename, "/dev/null") == 0)
filename = "NUL";
-# endif
+#endif
-# if OPEN_TRAILING_SLASH_BUG
+#if OPEN_TRAILING_SLASH_BUG
/* If the filename ends in a slash and one of O_CREAT, O_WRONLY, O_RDWR
is specified, then fail.
Rationale: POSIX <http://www.opengroup.org/susv3/basedefs/xbd_chap04.html>
@@ -97,11 +97,11 @@
return -1;
}
}
-# endif
+#endif
fd = orig_open (filename, flags, mode);
-# if OPEN_TRAILING_SLASH_BUG
+#if OPEN_TRAILING_SLASH_BUG
/* If the filename ends in a slash and fd does not refer to a directory,
then fail.
Rationale: POSIX <http://www.opengroup.org/susv3/basedefs/xbd_chap04.html>
@@ -129,13 +129,11 @@
}
}
}
-# endif
+#endif
+
#ifdef FCHDIR_REPLACEMENT
if (fd >= 0)
- {
- extern void _gl_register_fd (int, const char *);
- _gl_register_fd (fd, filename);
- }
+ _gl_register_fd (fd, filename);
#endif
return fd;
--- lib/winsock.c.orig 2008-10-09 02:51:10.000000000 +0200
+++ lib/winsock.c 2008-10-09 02:30:45.000000000 +0200
@@ -25,7 +25,6 @@
#include <io.h>
#include <sys/socket.h>
-#undef close
#undef socket
#undef connect
#undef accept
@@ -46,10 +45,9 @@
/* Hook for gnulib module close. */
-extern int _gl_close_fd (int fd);
-
+#if HAVE__GL_CLOSE_FD_MAYBE_SOCKET
int
-_gl_close_fd (int fd)
+_gl_close_fd_maybe_socket (int fd)
{
SOCKET sock = FD_TO_SOCKET (fd);
WSANETWORKEVENTS ev;
@@ -69,6 +67,7 @@
else
return _close (fd);
}
+#endif
/* Wrappers for WinSock functions. */
--- m4/close.m4.orig 2008-10-09 02:51:10.000000000 +0200
+++ m4/close.m4 2008-10-09 01:31:59.000000000 +0200
@@ -6,11 +6,12 @@
AC_DEFUN([gl_FUNC_CLOSE],
[
- AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
+ :
])
AC_DEFUN([gl_REPLACE_CLOSE],
[
+ AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
if test $REPLACE_CLOSE != 1; then
AC_LIBOBJ([close])
fi
--- m4/fchdir.m4.orig 2008-10-09 02:51:10.000000000 +0200
+++ m4/fchdir.m4 2008-10-09 02:13:34.000000000 +0200
@@ -1,5 +1,5 @@
-# fchdir.m4 serial 4
-dnl Copyright (C) 2006-2007 Free Software Foundation, Inc.
+# fchdir.m4 serial 5
+dnl Copyright (C) 2006-2008 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
dnl with or without modifications, as long as this notice is preserved.
@@ -7,22 +7,15 @@
AC_DEFUN([gl_FUNC_FCHDIR],
[
AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
- AC_REQUIRE([gl_FUNC_CLOSE])
- AC_REQUIRE([gl_FUNC_OPEN])
- AC_REQUIRE([gl_FUNC_FCHDIR_BODY])
-])
-
-AC_DEFUN([gl_FUNC_FCHDIR_BODY],
-[
AC_CHECK_FUNCS_ONCE([fchdir])
if test $ac_cv_func_fchdir = no; then
REPLACE_FCHDIR=1
AC_LIBOBJ([fchdir])
gl_PREREQ_FCHDIR
- gl_REPLACE_OPEN
- gl_REPLACE_CLOSE
AC_DEFINE([FCHDIR_REPLACEMENT], 1,
[Define if gnulib's fchdir() replacement is used.])
+ gl_REPLACE_OPEN
+ gl_REPLACE_CLOSE
gl_CHECK_NEXT_HEADERS([dirent.h])
DIRENT_H='dirent.h'
else
--- m4/open.m4.orig 2008-10-09 02:51:10.000000000 +0200
+++ m4/open.m4 2008-10-09 01:31:39.000000000 +0200
@@ -1,4 +1,4 @@
-# open.m4 serial 3
+# open.m4 serial 4
dnl Copyright (C) 2007-2008 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -51,6 +51,7 @@
AC_DEFUN([gl_REPLACE_OPEN],
[
+ AC_REQUIRE([gl_FCNTL_H_DEFAULTS])
if test $REPLACE_OPEN != 1; then
AC_LIBOBJ([open])
gl_PREREQ_OPEN
--- m4/sys_socket_h.m4.orig 2008-10-09 02:51:10.000000000 +0200
+++ m4/sys_socket_h.m4 2008-10-09 02:28:52.000000000 +0200
@@ -1,4 +1,4 @@
-# sys_socket_h.m4 serial 8
+# sys_socket_h.m4 serial 9
dnl Copyright (C) 2005-2008 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -71,6 +71,7 @@
# Sets and substitutes HAVE_WINSOCK2_H.
AC_DEFUN([gl_PREREQ_SYS_H_WINSOCK2],
[
+ AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
AC_CHECK_HEADERS_ONCE([sys/socket.h])
if test $ac_cv_header_sys_socket_h != yes; then
dnl We cannot use AC_CHECK_HEADERS_ONCE here, because that would make
@@ -81,6 +82,7 @@
fi
if test "$ac_cv_header_winsock2_h" = yes; then
HAVE_WINSOCK2_H=1
+ UNISTD_H_HAVE_WINSOCK2_H=1
else
HAVE_WINSOCK2_H=0
fi
@@ -97,6 +99,7 @@
AC_DEFUN([gl_SYS_SOCKET_H_DEFAULTS],
[
+ AC_REQUIRE([gl_UNISTD_H_DEFAULTS]) dnl for GNULIB_CLOSE
GNULIB_SOCKET=0; AC_SUBST([GNULIB_SOCKET])
GNULIB_CONNECT=0; AC_SUBST([GNULIB_CONNECT])
GNULIB_ACCEPT=0; AC_SUBST([GNULIB_ACCEPT])
--- m4/unistd_h.m4.orig 2008-10-09 02:51:10.000000000 +0200
+++ m4/unistd_h.m4 2008-10-09 01:58:35.000000000 +0200
@@ -1,4 +1,4 @@
-# unistd_h.m4 serial 13
+# unistd_h.m4 serial 14
dnl Copyright (C) 2006-2008 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -69,4 +69,5 @@
REPLACE_LCHOWN=0; AC_SUBST([REPLACE_LCHOWN])
REPLACE_LSEEK=0; AC_SUBST([REPLACE_LSEEK])
REPLACE_WRITE=0; AC_SUBST([REPLACE_WRITE])
+ UNISTD_H_HAVE_WINSOCK2_H=0; AC_SUBST([UNISTD_H_HAVE_WINSOCK2_H])
])
--- modules/accept.orig 2008-10-09 02:51:10.000000000 +0200
+++ modules/accept 2008-10-09 00:54:26.000000000 +0200
@@ -6,15 +6,12 @@
Depends-on:
sys_socket
-close
errno
configure.ac:
AC_REQUIRE([gl_HEADER_SYS_SOCKET])
-AC_REQUIRE([gl_FUNC_CLOSE])
if test "$ac_cv_header_winsock2_h" = yes; then
AC_LIBOBJ([winsock])
- gl_REPLACE_CLOSE
fi
gl_SYS_SOCKET_MODULE_INDICATOR([accept])
--- modules/close.orig 2008-10-09 02:51:10.000000000 +0200
+++ modules/close 2008-10-09 02:46:59.000000000 +0200
@@ -6,13 +6,16 @@
m4/close.m4
Depends-on:
+unistd
configure.ac:
gl_FUNC_CLOSE
+gl_UNISTD_MODULE_INDICATOR([close])
Makefile.am:
Include:
+<unistd.h>
License:
LGPLv2+
--- modules/poll-tests.orig 2008-10-09 02:51:10.000000000 +0200
+++ modules/poll-tests 2008-10-09 02:34:56.000000000 +0200
@@ -17,6 +17,7 @@
listen
connect
accept
+close
configure.ac:
AC_CHECK_HEADERS_ONCE([unistd.h sys/wait.h])
--- modules/select-tests.orig 2008-10-09 02:51:10.000000000 +0200
+++ modules/select-tests 2008-10-09 02:34:51.000000000 +0200
@@ -16,6 +16,7 @@
listen
connect
accept
+close
configure.ac:
--- modules/socket.orig 2008-10-09 02:51:10.000000000 +0200
+++ modules/socket 2008-10-09 00:54:43.000000000 +0200
@@ -6,15 +6,12 @@
Depends-on:
sys_socket
-close
errno
configure.ac:
AC_REQUIRE([gl_HEADER_SYS_SOCKET])
-AC_REQUIRE([gl_FUNC_CLOSE])
if test "$ac_cv_header_winsock2_h" = yes; then
AC_LIBOBJ([winsock])
- gl_REPLACE_CLOSE
fi
gl_SYS_SOCKET_MODULE_INDICATOR([socket])
--- modules/sys_socket.orig 2008-10-09 02:51:10.000000000 +0200
+++ modules/sys_socket 2008-10-09 02:39:48.000000000 +0200
@@ -6,6 +6,7 @@
lib/winsock.c
m4/sys_socket_h.m4
m4/sockpfaf.m4
+m4/unistd_h.m4
Depends-on:
include_next
@@ -14,6 +15,7 @@
configure.ac:
gl_HEADER_SYS_SOCKET
+gl_MODULE_INDICATOR([sys_socket])
AC_PROG_MKDIR_P
Makefile.am:
@@ -28,6 +30,7 @@
-e 's|@''PRAGMA_SYSTEM_HEADER''@|@PRAGMA_SYSTEM_HEADER@|g' \
-e 's|@''NEXT_SYS_SOCKET_H''@|$(NEXT_SYS_SOCKET_H)|g' \
-e 's|@''HAVE_SYS_SOCKET_H''@|$(HAVE_SYS_SOCKET_H)|g' \
+ -e 's|@''GNULIB_CLOSE''@|$(GNULIB_CLOSE)|g' \
-e 's|@''GNULIB_SOCKET''@|$(GNULIB_SOCKET)|g' \
-e 's|@''GNULIB_CONNECT''@|$(GNULIB_CONNECT)|g' \
-e 's|@''GNULIB_ACCEPT''@|$(GNULIB_ACCEPT)|g' \
--- modules/unistd.orig 2008-10-09 02:51:10.000000000 +0200
+++ modules/unistd 2008-10-09 01:59:56.000000000 +0200
@@ -60,6 +60,7 @@
-e 's|@''REPLACE_LCHOWN''@|$(REPLACE_LCHOWN)|g' \
-e 's|@''REPLACE_LSEEK''@|$(REPLACE_LSEEK)|g' \
-e 's|@''REPLACE_WRITE''@|$(REPLACE_WRITE)|g' \
+ -e 's|@''UNISTD_H_HAVE_WINSOCK2_H''@|$(UNISTD_H_HAVE_WINSOCK2_H)|g' \
-e '/definition of GL_LINK_WARNING/r $(LINK_WARNING_H)' \
< $(srcdir)/unistd.in.h; \
} > [EMAIL PROTECTED]
