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

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.