Re: [PATCH]: pselect: new module

[Bruno Haible]

Hi Paul,

> +/* Get definition of 'sigset_t'.
> +   But avoid namespace pollution on glibc systems.  */
> +#if !(defined __GLIBC__ && !defined __UCLIBC__)
> +# include <signal.h>
> +#endif

According to doc/posix-headers/signal.texi, it is not enough to
include <signal.h>. You also need add a module dependency from
'sys_select' to 'signal'.

Also, it would be useful to enhance tests/test-sys_select.c to verify
that sigset_t is indeed defined (since POSIX requests this type to be
defined in <sys/select.h>).

> +#if @GNULIB_PSELECT@
> +# if @REPLACE_PSELECT@
> +#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
> +#   undef pselect
> +#   define pselect rpl_pselect
> +#  endif
> +_GL_FUNCDECL_RPL (pselect, int,
> +                  (int, fd_set *restrict, fd_set *restrict, fd_set *restrict,
> +                   struct timespec const *restrict, const sigset_t 
> *restrict));
> +_GL_CXXALIAS_RPL (pselect, int,
> +                  (int, fd_set *restrict, fd_set *restrict, fd_set *restrict,
> +                   struct timespec const *restrict, const sigset_t 
> *restrict));
> +# else
> +#  if address@hidden@
> +_GL_FUNCDECL_SYS (pselect, int,
> +                  (int, fd_set *restrict, fd_set *restrict, fd_set *restrict,
> +                   struct timespec const *restrict, const sigset_t 
> *restrict));
> +#  endif
> +_GL_CXXALIAS_SYS (pselect, int,
> +                  (int, fd_set *restrict, fd_set *restrict, fd_set *restrict,
> +                   struct timespec const *restrict, const sigset_t 
> *restrict));

Good. But here also, it would be useful to enhance the unit test
tests/test-sys_select-c++.cc to verify that the function is declared
correctly in C++ mode.

> +# if HAVE_RAW_DECL_PSELECT
> +_GL_WARN_ON_USE (pselect, "pselect is not portable - "
> +                 "use gnulib module pselect for portability");

For HAVE_RAW_DECL_PSELECT to be defined, you need to augment the list of
functions to check in m4/sys_select_h.m4 line 67 (the argument of
gl_WARN_ON_USE_PREPARE).

> +    AC_CACHE_CHECK([whether signature of pselect conforms to POSIX],
> +      gl_cv_sig_pselect,
> +      [AC_LINK_IFELSE(
> +      [AC_LANG_PROGRAM(
> +           [[#include <sys/select.h>
> +             ]],
> +           [[int (*p) (int, fd_set *, fd_set *, fd_set *restrict,
> +                       struct timespec const *restrict,
> +                       sigset_t const *restrict) = pselect;
> +             return !p;]])],

Tabs are unwelcome in gnulib source files, because they make the indentation
look incorrect.

> +  if (sigmask)
> +    sigprocmask (SIG_SETMASK, sigmask, &origmask);

If the emulation uses sigprocmask, the module needs to depend on module
'sigprocmask'. See doc/posix-functions/sigprocmask.texi.

> +configure.ac:
> +gl_FUNC_PSELECT
> +if test $REPLACE_PSELECT = 1; then
> +  AC_LIBOBJ([pselect])
> +fi

The condition ought to be
    test $HAVE_PSELECT = 0 || test $REPLACE_PSELECT = 1
like for many other functions (see e.g. modules/nanosleep).

> +Link:
> +

The replacement is based on the select() function, and the 'select' module
requires linking with $(LIBSOCKET). The 'pselect' module therefore needs to
mention the same link requirement.

> I've tested it on Solaris 8 (!) and Fedora 14 for starters.

How did you test it? I don't see any unit test. Without a unit test, you
cannot even guarantee that the function is defined on platforms which
lack it (which is probably why you didn't notice the missing
'test $HAVE_PSELECT = 0').

For the unit tests, I would copy and adapt tests/test-select*.

Last not least, the documentation: doc/posix-functions/pselect.texi has not
been modified.

Bruno
-- 
In memoriam Yuri Shchekochikhin 
<http://en.wikipedia.org/wiki/Yuri_Shchekochikhin>