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

[Daniel P. Berrangé]

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.

I'd initialize 'struct pollfd' inside the loop, such that
it only ever contains valid open FDs.

>  
> -    if (cmd->outfd >= 0 && FD_ISSET (cmd->outfd, &rset2)) {
> +    if (fds[1].revents & POLLIN) {
>        /* Read the output, buffer it up to the end of the line, then
>         * pass it to the callback.
>         */
> @@ -716,18 +725,27 @@ loop (struct command *cmd)
>            cmd->outbuf.close_data (cmd);
>          if (close (cmd->outfd) == -1)
>            perrorf (cmd->g, "close: outfd");
> -        FD_CLR (cmd->outfd, &rset);
> +        fds[1].fd = -1;
>          cmd->outfd = -1;
>          nr_fds--;
>        }
>        else if (n == -1) {
>          perrorf (cmd->g, "read: outfd");
>          close (cmd->outfd);
> -        FD_CLR (cmd->outfd, &rset);
> +        fds[1].fd = -1;
>          cmd->outfd = -1;
>          nr_fds--;
>        }
>      }
> +    if (fds[1].revents & POLLHUP) {
> +      if (cmd->outbuf.close_data)
> +        cmd->outbuf.close_data (cmd);
> +      if (close (cmd->outfd) == -1)
> +        perrorf (cmd->g, "close: outfd");
> +      fds[1].fd = -1;
> +      cmd->outfd = -1;
> +      nr_fds--;
> +    }
>    }
>  
>    return 0;
> -- 
> 2.24.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|