thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH] lib: command: switch from select() to poll() [Feb 2020]

If this information is useful, please help other people find it:
Share via:

Pino Toscano

2020-Feb-26 13:39 UTC

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

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--;
+    }
 
-    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

Daniel P. Berrangé

2020-Feb-26 14:08 UTC

head link

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.

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@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 :|

Pino Toscano

2020-Feb-26 14:14 UTC

head link

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

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.

-- 
Pino Toscano

Possibly Parallel Threads

Search for more reasonably related threads

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

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

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

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

Possibly Parallel Threads