[Libguestfs] [PATCH] lib: command: switch from select() to poll()

[Richard W.M. Jones]

On Wed, Feb 26, 2020 at 03:14:37PM +0100, Pino Toscano wrote:
> On Wednesday, 26 February 2020 15:08:24 CET Daniel P. Berrangé wrote:
> > On Wed, Feb 26, 2020 at 02:39:04PM +0100, Pino Toscano wrote:
> > > select() has a maximum value for the FDs it can monitor, and since
> > > the libguestfs library can be used in other applications, this limit
> > > may be hit by users in case lots of FDs are opened.
> > > 
> > > As solution, switch to poll(): it has a slightly better interface to
> > > check what changed and for which FD, and it does not have a limit in the
> > > value of the FDs monitored.
> > > 
> > > poll() is supported on the platforms we support, so there is no need to
> > > use the gnulib module for it.
> > > ---
> > >  lib/command.c | 54 ++++++++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 36 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/lib/command.c b/lib/command.c
> > > index f2161de9a..13b084934 100644
> > > --- a/lib/command.c
> > > +++ b/lib/command.c
> > > @@ -89,9 +89,8 @@
> > >  #include <signal.h>
> > >  #include <errno.h>
> > >  #include <assert.h>
> > > -#include <sys/types.h>
> > >  #include <sys/stat.h>
> > > -#include <sys/select.h>
> > > +#include <poll.h>
> > >  
> > >  #ifdef HAVE_SYS_TIME_H
> > >  #include <sys/time.h>
> > > @@ -650,37 +649,41 @@ run_child (struct command *cmd)
> > >  static int
> > >  loop (struct command *cmd)
> > >  {
> > > -  fd_set rset, rset2;
> > > -  int maxfd = -1, r;
> > > +  struct pollfd fds[2];
> > > +  int r;
> > >    size_t nr_fds = 0;
> > >    CLEANUP_FREE char *buf = safe_malloc (cmd->g, BUFSIZ);
> > >    ssize_t n;
> > >  
> > > -  FD_ZERO (&rset);
> > > +  memset (&fds, 0, sizeof fds);
> > > +
> > > +  fds[0].fd = cmd->errorfd;
> > > +  fds[1].fd = cmd->outfd;
> > >  
> > >    if (cmd->errorfd >= 0) {
> > > -    FD_SET (cmd->errorfd, &rset);
> > > -    maxfd = MAX (cmd->errorfd, maxfd);
> > > +    fds[0].events = POLLIN;
> > >      nr_fds++;
> > >    }
> > >  
> > >    if (cmd->outfd >= 0) {
> > > -    FD_SET (cmd->outfd, &rset);
> > > -    maxfd = MAX (cmd->outfd, maxfd);
> > > +    fds[1].events = POLLIN;
> > >      nr_fds++;
> > >    }
> > >  
> > >    while (nr_fds > 0) {
> > > -    rset2 = rset;
> > > -    r = select (maxfd+1, &rset2, NULL, NULL, NULL);
> > > +    r = poll (fds, 2, -1);
> > >      if (r == -1) {
> > >        if (errno == EINTR || errno == EAGAIN)
> > >          continue;
> > > -      perrorf (cmd->g, "select");
> > > +      perrorf (cmd->g, "poll");
> > > +      return -1;
> > > +    }
> > > +    if (fds[0].revents & POLLERR || fds[1].revents & POLLERR) {
> > > +      perrorf (cmd->g, "poll");
> > >        return -1;
> > >      }
> > >  
> > > -    if (cmd->errorfd >= 0 && FD_ISSET (cmd->errorfd, &rset2)) {
> > > +    if (fds[0].revents & POLLIN) {
> > >        /* Read output and send it to the log. */
> > >        n = read (cmd->errorfd, buf, BUFSIZ);
> > >        if (n > 0)
> > > @@ -689,20 +692,26 @@ loop (struct command *cmd)
> > >        else if (n == 0) {
> > >          if (close (cmd->errorfd) == -1)
> > >            perrorf (cmd->g, "close: errorfd");
> > > -        FD_CLR (cmd->errorfd, &rset);
> > > +        fds[0].fd = -1;
> > >          cmd->errorfd = -1;
> > >          nr_fds--;
> > >        }
> > >        else if (n == -1) {
> > >          perrorf (cmd->g, "read: errorfd");
> > >          close (cmd->errorfd);
> > > -        FD_CLR (cmd->errorfd, &rset);
> > > +        fds[0].fd = -1;
> > >          cmd->errorfd = -1;
> > >          nr_fds--;
> > >        }
> > >      }
> > > +    if (fds[0].revents & POLLHUP) {
> > > +      close (cmd->errorfd);
> > > +      fds[0].fd = -1;
> > > +      cmd->errorfd = -1;
> > > +      nr_fds--;
> > > +    }
> > 
> > Now fds[0] is -1, and fds[1] is still an open file
> > descriptor.
> > 
> > Next time around the loop it will call poll() on 'fds' array
> > which contains this FD == -1. In theory that results in POLLNVAL,
> > but on OS-X at least it causes poll() itself to return an error.
> 
> Hmm in the documentations there are these bits:
> 
> http://man7.org/linux/man-pages/man2/poll.2.html
> "The  field fd contains a file descriptor for an open file.  If this
> field is negative, then the corresponding events field is ignored and
> the revents field returns zero.  (This provides an easy way of ignoring
> a file descriptor for a single poll() call: simply negate the fd field.
> Note, however, that this technique can't be used to ignore file
> descriptor 0.)"
> 
> https://linux.die.net/man/3/poll
> "If the value of fd is less than 0, events shall be ignored, and revents
> shall be set to 0 in that entry on return from poll()."
> 
> So to my understanding it is fine, and indeed I did not get POLLNVAL
> in my testing.

In fact libnbd & nbdkit also rely on this behaviour.  I don't believe
we've ever tried either one on macOS though.  It seems to be mandated
by POSIX too:

https://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html

  "If the value of fd is less than 0, events shall be ignored, and
  revents shall be set to 0 in that entry on return from poll()."

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org