Re: [Bug-gnulib] Module: poll

[Paul Eggert]

"Bonzini" <address@hidden> writes:

> Is it ok, or is there anything I should fix in the implementation
> before writing the autoconf tests

Here are some ideas and suggestions.

gnulib <poll.h> should not assume that a working system <poll.h>
merely includes <sys/poll.h>.  I would install into gnulib a file
whose name is (say) "poll_.h"; this file should be linked or copied to
"poll.h" by "configure" only if the system doesn't have a working
"poll.h".  See fnmatch in gnulib for an example of this.  If you do
this, gnulib's poll_.h does not need to include <sys/poll.h>.

Second, the poll emulation should support everything required by the
POSIX standard for poll.  See:

http://www.opengroup.org/onlinepubs/007904975/basedefs/poll.h.html
http://www.opengroup.org/onlinepubs/007904975/functions/poll.html

I noticed the following problems when I briefly compared the proposed
poll.h and poll.c to the POSIX standard.

In poll.h:

* nfds_t is not defined.  I suggest "typedef unsigned long nfds_t;",
  as it's typical.
* 'poll' is not declared.
* The symbol INFTIM is defined, but it shouldn't be present.
* config.h is included, but it isn't needed by poll.h; it should be removed.

In poll.c:

* poll's local variable 'i' should be of type nfds_t.
* poll doesn't fail with errno == EINVAL if nfd exceeds {OPEN_MAX}.
* poll has undefined behavior if one of the file descriptors is
  greater than FD_SETSIZE.  I'd say it should fail with errno==EOVERFLOW
  in that situation (or EINVAL if there is no EOVERFLOW).

* The code "if (maxfd == -1) { errno = EINVAL; return -1; }" doesn't
  seem right to me.  It's OK to invoke poll with no file descriptors
  and with a timeout.

* This code doesn't seem conforming to me.  POSIX says that revents
  should be 0 in this case.

      if (pfd[i].fd < 0) {
        pfd[i].revents |= POLLNVAL;
        continue;
      }

Very minor code improvements:

* You can change "timeout > 0" to "timeout >= 0" and remove the
  "timeout == 0" case.
* "ok" can be a boolean, and you can then replace "if (ok) rc++;" with
  "rc += ok;".

Also, both poll.c and poll.h say that the files are part of GNU
Smalltalk, but I suppose this part of the copyright notice should be
removed if the files are intended to be generic parts of gnulib.

I'll second Bruno's suggestion to fix the indenting style.  Simplest
may be GNU "indent".