Xen devel - Feb 2013 - [PATCH] mini-os: implement poll(2)

It is just a wrapper around select(2).

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 extras/mini-os/include/posix/poll.h |    1 +
 extras/mini-os/lib/sys.c            |   90 ++++++++++++++++++++++++++++++++++-
 2 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100644 extras/mini-os/include/posix/poll.h

diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
new file mode 100644
index 0000000..06fb41a
--- /dev/null
+++ b/extras/mini-os/include/posix/poll.h
@@ -0,0 +1 @@
+#include <sys/poll.h>
diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
index 3cc3340..aae02df 100644
--- a/extras/mini-os/lib/sys.c
+++ b/extras/mini-os/lib/sys.c
@@ -31,6 +31,7 @@
 #include <tpm_tis.h>
 #include <xenbus.h>
 #include <xenstore.h>
+#include <poll.h>
 
 #include <sys/types.h>
 #include <sys/unistd.h>
@@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *except
 #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
 #endif
 
+#ifdef LIBC_DEBUG
+static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
+{
+    int i, comma, fd;
+
+    printk("[");
+    comma = 0;
+    for (i = 0; i < nfds; i++) {
+        fd = pfd[i].fd;
+        if (comma)
+            printk(", ");
+        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
+            pfd[i].events);
+            comma = 1;
+    }
+    printk("]");
+
+    printk(", %d, %d", nfds, timeout);
+}
+#else
+#define dump_pollfds(pfds, nfds, timeout)
+#endif
+
 /* Just poll without blocking */
 static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set
*exceptfds)
 {
@@ -983,6 +1007,71 @@ out:
     return ret;
 }
 
+/* Wrap around select */
+int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
+{
+    int ret;
+    int i, fd;
+    struct timeval _timeo, *timeo = NULL;
+    fd_set rfds, wfds, efds;
+    int max_fd = -1;
+
+    DEBUG("poll(");
+    dump_pollfds(_pfd, _nfds, _timeout);
+    DEBUG(")\n");
+
+    if (_timeout != -1) {
+        /* Timeout in poll is in millisecond. */
+        _timeo.tv_sec  = _timeout / 1000;
+        _timeo.tv_usec = (_timeout - _timeo.tv_sec * 1000) * 1000;
+        timeo = &_timeo;
+    }
+
+    FD_ZERO(&rfds);
+    FD_ZERO(&wfds);
+    FD_ZERO(&efds);
+
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+        /* map POLL* into readfds and writefds */
+        /* POLLIN  -> readfds
+         * POLLOUT -> writefds
+         * POLL*   -> none
+         */
+        if (_pfd[i].events & POLLIN)
+            FD_SET(fd, &rfds);
+        if (_pfd[i].events & POLLOUT)
+            FD_SET(fd, &wfds);
+        /* always set exceptfds */
+        FD_SET(fd, &efds);
+        _pfd[i].revents = 0;
+        if (fd > max_fd)
+            max_fd = fd;
+    }
+
+    ret = select(max_fd+1, &rfds, &wfds, &efds, timeo);
+
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+        if (FD_ISSET(fd, &efds)) {
+             /* anything bad happens we set POLLERR, ignoring PULLHUP
+              * POLLNVAL */
+            _pfd[i].revents |= POLLERR;
+            continue;
+        }
+        if (FD_ISSET(fd, &rfds))
+            _pfd[i].revents |= POLLIN;
+        if (FD_ISSET(fd, &wfds))
+            _pfd[i].revents |= POLLOUT;
+    }
+
+    DEBUG("poll(");
+    dump_pollfds(_pfd, _nfds, _timeout);
+    DEBUG(")\n");
+
+    return ret;
+}
+
 #ifdef HAVE_LWIP
 int socket(int domain, int type, int protocol)
 {
@@ -1360,7 +1449,6 @@ unsupported_function(int, tcgetattr, 0);
 unsupported_function(int, grantpt, -1);
 unsupported_function(int, unlockpt, -1);
 unsupported_function(char *, ptsname, NULL);
-unsupported_function(int, poll, -1);
 
 /* net/if.h */
 unsupported_function_log(unsigned int, if_nametoindex, -1);
-- 
1.7.10.4

On Tue, 2013-02-19 at 13:41 +0000, Wei Liu wrote:
> It is just a wrapper around select(2).
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  extras/mini-os/include/posix/poll.h |    1 +
>  extras/mini-os/lib/sys.c            |   90
++++++++++++++++++++++++++++++++++-
>  2 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 extras/mini-os/include/posix/poll.h
> 
> diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
> new file mode 100644
> index 0000000..06fb41a
> --- /dev/null
> +++ b/extras/mini-os/include/posix/poll.h
> @@ -0,0 +1 @@
> +#include <sys/poll.h>
> diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
> index 3cc3340..aae02df 100644
> --- a/extras/mini-os/lib/sys.c
> +++ b/extras/mini-os/lib/sys.c
> @@ -31,6 +31,7 @@
>  #include <tpm_tis.h>
>  #include <xenbus.h>
>  #include <xenstore.h>
> +#include <poll.h>
>  
>  #include <sys/types.h>
>  #include <sys/unistd.h>
> @@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *except
>  #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
>  #endif
>  
> +#ifdef LIBC_DEBUG
> +static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
> +{
> +    int i, comma, fd;
> +
> +    printk("[");
> +    comma = 0;
> +    for (i = 0; i < nfds; i++) {
> +        fd = pfd[i].fd;
> +        if (comma)
> +            printk(", ");
> +        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
> +            pfd[i].events);
> +            comma = 1;
> +    }
> +    printk("]");
> +
> +    printk(", %d, %d", nfds, timeout);
> +}
> +#else
> +#define dump_pollfds(pfds, nfds, timeout)
> +#endif
> +
>  /* Just poll without blocking */
>  static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set
*exceptfds)
>  {
> @@ -983,6 +1007,71 @@ out:
>      return ret;
>  }
>  
> +/* Wrap around select */
> +int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
> +{
> +    int ret;
> +    int i, fd;
> +    struct timeval _timeo, *timeo = NULL;
> +    fd_set rfds, wfds, efds;
> +    int max_fd = -1;
> +
> +    DEBUG("poll(");
> +    dump_pollfds(_pfd, _nfds, _timeout);
> +    DEBUG(")\n");
> +
> +    if (_timeout != -1) {
should be _timeout >= 0, any negative value will cause an infinite wait.
> +        /* Timeout in poll is in millisecond. */
> +        _timeo.tv_sec  = _timeout / 1000;
> +        _timeo.tv_usec = (_timeout - _timeo.tv_sec * 1000) * 1000;
why not _timeout % 1000? gcc can also optimize and detect you have a
division and a module and use only a single instruction in x86.
> +        timeo = &_timeo;
> +    }
> +
> +    FD_ZERO(&rfds);
> +    FD_ZERO(&wfds);
> +    FD_ZERO(&efds);
> +
> +    for (i = 0; i < _nfds; i++) {
> +        fd = _pfd[i].fd;
I think you should check if fd is < 0 and return EINVAL (not sure about
the error code). Well.. probably you should check fd in range 0..1023.
> +        /* map POLL* into readfds and writefds */
> +        /* POLLIN  -> readfds
> +         * POLLOUT -> writefds
> +         * POLL*   -> none
> +         */
> +        if (_pfd[i].events & POLLIN)
> +            FD_SET(fd, &rfds);
> +        if (_pfd[i].events & POLLOUT)
> +            FD_SET(fd, &wfds);
> +        /* always set exceptfds */
> +        FD_SET(fd, &efds);
> +        _pfd[i].revents = 0;
> +        if (fd > max_fd)
> +            max_fd = fd;
> +    }
> +
> +    ret = select(max_fd+1, &rfds, &wfds, &efds, timeo);
> +
> +    for (i = 0; i < _nfds; i++) {
> +        fd = _pfd[i].fd;
> +        if (FD_ISSET(fd, &efds)) {
> +             /* anything bad happens we set POLLERR, ignoring PULLHUP
> +              * POLLNVAL */
> +            _pfd[i].revents |= POLLERR;
> +            continue;
> +        }
> +        if (FD_ISSET(fd, &rfds))
> +            _pfd[i].revents |= POLLIN;
> +        if (FD_ISSET(fd, &wfds))
> +            _pfd[i].revents |= POLLOUT;
> +    }
> +
> +    DEBUG("poll(");
> +    dump_pollfds(_pfd, _nfds, _timeout);
> +    DEBUG(")\n");
> +
you probably want to save and restore errno or are we sure dump_pollfds
cannot overwrite errno ?
> +    return ret;
> +}
> +
>  #ifdef HAVE_LWIP
>  int socket(int domain, int type, int protocol)
>  {
> @@ -1360,7 +1449,6 @@ unsupported_function(int, tcgetattr, 0);
>  unsupported_function(int, grantpt, -1);
>  unsupported_function(int, unlockpt, -1);
>  unsupported_function(char *, ptsname, NULL);
> -unsupported_function(int, poll, -1);
>  
>  /* net/if.h */
>  unsupported_function_log(unsigned int, if_nametoindex, -1);
Frediano

On Tue, 2013-02-19 at 14:06 +0000, Frediano Ziglio
wrote:
> On Tue, 2013-02-19 at 13:41 +0000, Wei Liu wrote:
> > It is just a wrapper around select(2).
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  extras/mini-os/include/posix/poll.h |    1 +
> >  extras/mini-os/lib/sys.c            |   90
++++++++++++++++++++++++++++++++++-
> >  2 files changed, 90 insertions(+), 1 deletion(-)
> >  create mode 100644 extras/mini-os/include/posix/poll.h
> > 
> > diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
> > new file mode 100644
> > index 0000000..06fb41a
> > --- /dev/null
> > +++ b/extras/mini-os/include/posix/poll.h
> > @@ -0,0 +1 @@
> > +#include <sys/poll.h>
> > diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
> > index 3cc3340..aae02df 100644
> > --- a/extras/mini-os/lib/sys.c
> > +++ b/extras/mini-os/lib/sys.c
> > @@ -31,6 +31,7 @@
> >  #include <tpm_tis.h>
> >  #include <xenbus.h>
> >  #include <xenstore.h>
> > +#include <poll.h>
> >  
> >  #include <sys/types.h>
> >  #include <sys/unistd.h>
> > @@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set *readfds,
fd_set *writefds, fd_set *except
> >  #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
> >  #endif
> >  
> > +#ifdef LIBC_DEBUG
> > +static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
> > +{
> > +    int i, comma, fd;
> > +
> > +    printk("[");
> > +    comma = 0;
> > +    for (i = 0; i < nfds; i++) {
> > +        fd = pfd[i].fd;
> > +        if (comma)
> > +            printk(", ");
> > +        printk("%d(%c)/%02x", fd,
file_types[files[fd].type],
> > +            pfd[i].events);
> > +            comma = 1;
> > +    }
> > +    printk("]");
> > +
> > +    printk(", %d, %d", nfds, timeout);
> > +}
> > +#else
> > +#define dump_pollfds(pfds, nfds, timeout)
> > +#endif
> > +
> >  /* Just poll without blocking */
> >  static int select_poll(int nfds, fd_set *readfds, fd_set *writefds,
fd_set *exceptfds)
> >  {
> > @@ -983,6 +1007,71 @@ out:
> >      return ret;
> >  }
> >  
> > +/* Wrap around select */
> > +int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
> > +{
> > +    int ret;
> > +    int i, fd;
> > +    struct timeval _timeo, *timeo = NULL;
> > +    fd_set rfds, wfds, efds;
> > +    int max_fd = -1;
> > +
> > +    DEBUG("poll(");
> > +    dump_pollfds(_pfd, _nfds, _timeout);
> > +    DEBUG(")\n");
> > +
> > +    if (_timeout != -1) {
> 
> should be _timeout >= 0, any negative value will cause an infinite wait.
> 
Right, I was too obsessed with xenstore code which used -1 to represent
infinity loop.
> > +        /* Timeout in poll is in millisecond. */
> > +        _timeo.tv_sec  = _timeout / 1000;
> > +        _timeo.tv_usec = (_timeout - _timeo.tv_sec * 1000) * 1000;
> 
> why not _timeout % 1000? gcc can also optimize and detect you have a
> division and a module and use only a single instruction in x86.
> 
Done.
> > +        timeo = &_timeo;
> > +    }
> > +
> > +    FD_ZERO(&rfds);
> > +    FD_ZERO(&wfds);
> > +    FD_ZERO(&efds);
> > +
> > +    for (i = 0; i < _nfds; i++) {
> > +        fd = _pfd[i].fd;
> 
> I think you should check if fd is < 0 and return EINVAL (not sure about
> the error code). Well.. probably you should check fd in range 0..1023.
> 
Unfortunately this is not specified in the standard. I guess we can just
ignore print a warning and ignore this fd?

http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html#tag_16_423
> > +
> > +    DEBUG("poll(");
> > +    dump_pollfds(_pfd, _nfds, _timeout);
> > +    DEBUG(")\n");
> > +
> 
> you probably want to save and restore errno or are we sure dump_pollfds
> cannot overwrite errno ?
> 
Done.


Wei.

New version of the patch. Address Frediano''s comments, also correct the
calculation of return value.

Wei.

----8<----
From 4b7dffa7b61ff840a359f7124c76198b7aeed135 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Tue, 19 Feb 2013 12:15:03 +0000
Subject: [PATCH] mini-os: implement poll(2)

It is just a wrapper around select(2).

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 extras/mini-os/include/posix/poll.h |    1 +
 extras/mini-os/lib/sys.c            |  101 ++++++++++++++++++++++++++++++++++-
 2 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 extras/mini-os/include/posix/poll.h

diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
new file mode 100644
index 0000000..06fb41a
--- /dev/null
+++ b/extras/mini-os/include/posix/poll.h
@@ -0,0 +1 @@
+#include <sys/poll.h>
diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
index 3cc3340..d85aa38 100644
--- a/extras/mini-os/lib/sys.c
+++ b/extras/mini-os/lib/sys.c
@@ -31,6 +31,7 @@
 #include <tpm_tis.h>
 #include <xenbus.h>
 #include <xenstore.h>
+#include <poll.h>
 
 #include <sys/types.h>
 #include <sys/unistd.h>
@@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *except
 #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
 #endif
 
+#ifdef LIBC_DEBUG
+static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
+{
+    int i, comma, fd;
+
+    printk("[");
+    comma = 0;
+    for (i = 0; i < nfds; i++) {
+        fd = pfd[i].fd;
+        if (comma)
+            printk(", ");
+        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
+            pfd[i].events);
+            comma = 1;
+    }
+    printk("]");
+
+    printk(", %d, %d", nfds, timeout);
+}
+#else
+#define dump_pollfds(pfds, nfds, timeout)
+#endif
+
 /* Just poll without blocking */
 static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set
*exceptfds)
 {
@@ -983,6 +1007,82 @@ out:
     return ret;
 }
 
+/* Wrap around select */
+int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
+{
+    int ret;
+    int i, fd;
+    struct timeval _timeo, *timeo = NULL;
+    fd_set rfds, wfds, efds;
+    int max_fd = -1;
+    int saved_errno;
+
+    DEBUG("poll(");
+    dump_pollfds(_pfd, _nfds, _timeout);
+    DEBUG(")\n");
+
+    if (_timeout >= 0) {
+        /* Timeout in poll is in millisecond. */
+        _timeo.tv_sec  = _timeout / 1000;
+        _timeo.tv_usec = (_timeout % 1000) * 1000;
+        timeo = &_timeo;
+    }
+
+    FD_ZERO(&rfds);
+    FD_ZERO(&wfds);
+    FD_ZERO(&efds);
+
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+        if (fd < 0 || fd > 1023) {
+            DEBUG("trying to poll fd %d which is not between
[0,1023]\n", fd);
+            continue;
+        }  
+        /* map POLL* into readfds and writefds */
+        /* POLLIN  -> readfds
+         * POLLOUT -> writefds
+         * POLL*   -> none
+         */
+        if (_pfd[i].events & POLLIN)
+            FD_SET(fd, &rfds);
+        if (_pfd[i].events & POLLOUT)
+            FD_SET(fd, &wfds);
+        /* always set exceptfds */
+        FD_SET(fd, &efds);
+        _pfd[i].revents = 0;
+        if (fd > max_fd)
+            max_fd = fd;
+    }
+
+    /* don''t care about the return value, but we do need errno */
+    select(max_fd+1, &rfds, &wfds, &efds, timeo);
+
+    ret = 0;
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+        if (FD_ISSET(fd, &rfds) || FD_ISSET(fd, &wfds) || FD_ISSET(fd,
&efds))
+            ret++;
+        if (FD_ISSET(fd, &efds)) {
+             /* anything bad happens we set POLLERR, ignoring PULLHUP
+              * POLLNVAL */
+            _pfd[i].revents |= POLLERR;
+            continue;
+        }
+        if (FD_ISSET(fd, &rfds))
+            _pfd[i].revents |= POLLIN;
+        if (FD_ISSET(fd, &wfds))
+            _pfd[i].revents |= POLLOUT;
+    }
+
+    saved_errno = errno;
+    DEBUG("poll(");
+    dump_pollfds(_pfd, _nfds, _timeout);
+    DEBUG(")\n");
+    errno = saved_errno;
+
+    return ret;
+}
+
 #ifdef HAVE_LWIP
 int socket(int domain, int type, int protocol)
 {
@@ -1360,7 +1460,6 @@ unsupported_function(int, tcgetattr, 0);
 unsupported_function(int, grantpt, -1);
 unsupported_function(int, unlockpt, -1);
 unsupported_function(char *, ptsname, NULL);
-unsupported_function(int, poll, -1);
 
 /* net/if.h */
 unsupported_function_log(unsigned int, if_nametoindex, -1);
-- 
1.7.10.4

On Tue, 2013-02-19 at 14:22 +0000, Wei Liu wrote:
> On Tue, 2013-02-19 at 14:06 +0000, Frediano Ziglio wrote:
> > On Tue, 2013-02-19 at 13:41 +0000, Wei Liu wrote:
> > > It is just a wrapper around select(2).
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  extras/mini-os/include/posix/poll.h |    1 +
> > >  extras/mini-os/lib/sys.c            |   90
++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 90 insertions(+), 1 deletion(-)
> > >  create mode 100644 extras/mini-os/include/posix/poll.h
> > > 
> > > diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
> > > new file mode 100644
> > > index 0000000..06fb41a
> > > --- /dev/null
> > > +++ b/extras/mini-os/include/posix/poll.h
> > > @@ -0,0 +1 @@
> > > +#include <sys/poll.h>
> > > diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
> > > index 3cc3340..aae02df 100644
> > > --- a/extras/mini-os/lib/sys.c
> > > +++ b/extras/mini-os/lib/sys.c
> > > @@ -31,6 +31,7 @@
> > >  #include <tpm_tis.h>
> > >  #include <xenbus.h>
> > >  #include <xenstore.h>
> > > +#include <poll.h>
> > >  
> > >  #include <sys/types.h>
> > >  #include <sys/unistd.h>
> > > @@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set
*readfds, fd_set *writefds, fd_set *except
> > >  #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
> > >  #endif
> > >  
> > > +#ifdef LIBC_DEBUG
> > > +static void dump_pollfds(struct pollfd *pfd, int nfds, int
timeout)
> > > +{
> > > +    int i, comma, fd;
> > > +
> > > +    printk("[");
> > > +    comma = 0;
> > > +    for (i = 0; i < nfds; i++) {
> > > +        fd = pfd[i].fd;
> > > +        if (comma)
> > > +            printk(", ");
> > > +        printk("%d(%c)/%02x", fd,
file_types[files[fd].type],
> > > +            pfd[i].events);
> > > +            comma = 1;
> > > +    }
> > > +    printk("]");
> > > +
> > > +    printk(", %d, %d", nfds, timeout);
> > > +}
> > > +#else
> > > +#define dump_pollfds(pfds, nfds, timeout)
> > > +#endif
> > > +
> > >  /* Just poll without blocking */
> > >  static int select_poll(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *exceptfds)
> > >  {
> > > @@ -983,6 +1007,71 @@ out:
> > >      return ret;
> > >  }
> > >  
> > > +/* Wrap around select */
> > > +int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
> > > +{
> > > +    int ret;
> > > +    int i, fd;
> > > +    struct timeval _timeo, *timeo = NULL;
> > > +    fd_set rfds, wfds, efds;
> > > +    int max_fd = -1;
> > > +
> > > +    DEBUG("poll(");
> > > +    dump_pollfds(_pfd, _nfds, _timeout);
> > > +    DEBUG(")\n");
> > > +
> > > +    if (_timeout != -1) {
> > 
> > should be _timeout >= 0, any negative value will cause an infinite
wait.
> > 
> 
> Right, I was too obsessed with xenstore code which used -1 to represent
> infinity loop.
> 
> > > +        /* Timeout in poll is in millisecond. */
> > > +        _timeo.tv_sec  = _timeout / 1000;
> > > +        _timeo.tv_usec = (_timeout - _timeo.tv_sec * 1000) *
1000;
> > 
> > why not _timeout % 1000? gcc can also optimize and detect you have a
> > division and a module and use only a single instruction in x86.
> > 
> 
> Done.
> 
> > > +        timeo = &_timeo;
> > > +    }
> > > +
> > > +    FD_ZERO(&rfds);
> > > +    FD_ZERO(&wfds);
> > > +    FD_ZERO(&efds);
> > > +
> > > +    for (i = 0; i < _nfds; i++) {
> > > +        fd = _pfd[i].fd;
> > 
> > I think you should check if fd is < 0 and return EINVAL (not sure
about
> > the error code). Well.. probably you should check fd in range 0..1023.
> > 
> 
> Unfortunately this is not specified in the standard. I guess we can just
> ignore print a warning and ignore this fd?
> 
>
http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html#tag_16_423
> 
Yes, it speaks only of STREAM (I don''t know what''s in this
context).

I tried what happen on Linux. For values < 0 if just ignore the record.
Oh... from page you send

"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()."

For values that are not open if return POLLNVAL in revents.

Perhaps should be something like

    ret = 0;
    for (i = 0; i < _nfds; i++) {
        fd = _pfd[i].fd;
        _pfd[i].revents = 0;
        if (fd < 0) continue;
        if (fd >= 1024) {
            _pfd[i].revents = POLLNVAL;
            ++ret;
            continue;
        }
        /* map POLL* into readfds and writefds */
        /* POLLIN  -> readfds
         * POLLOUT -> writefds
         * POLL*   -> none
         */
        if (_pfd[i].events & POLLIN)
            FD_SET(fd, &rfds);
        if (_pfd[i].events & POLLOUT)
            FD_SET(fd, &wfds);
        /* always set exceptfds */
        FD_SET(fd, &efds);
        if (fd > max_fd)
            max_fd = fd;
    }
    if (ret) return ret;
> > > +
> > > +    DEBUG("poll(");
> > > +    dump_pollfds(_pfd, _nfds, _timeout);
> > > +    DEBUG(")\n");
> > > +
> > 
> > you probably want to save and restore errno or are we sure
dump_pollfds
> > cannot overwrite errno ?
> > 
> 
> Done.
> 
> 
> Wei.
> 
>

On Tue, 2013-02-19 at 15:21 +0000, Frediano Ziglio
wrote:
> On Tue, 2013-02-19 at 14:22 +0000, Wei Liu wrote:
> > On Tue, 2013-02-19 at 14:06 +0000, Frediano Ziglio wrote:
> > > On Tue, 2013-02-19 at 13:41 +0000, Wei Liu wrote:
> > > > It is just a wrapper around select(2).
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > >  extras/mini-os/include/posix/poll.h |    1 +
> > > >  extras/mini-os/lib/sys.c            |   90
++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 90 insertions(+), 1 deletion(-)
> > > >  create mode 100644 extras/mini-os/include/posix/poll.h
> > > > 
> > > > diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
> > > > new file mode 100644
> > > > index 0000000..06fb41a
> > > > --- /dev/null
> > > > +++ b/extras/mini-os/include/posix/poll.h
> > > > @@ -0,0 +1 @@
> > > > +#include <sys/poll.h>
> > > > diff --git a/extras/mini-os/lib/sys.c
b/extras/mini-os/lib/sys.c
> > > > index 3cc3340..aae02df 100644
> > > > --- a/extras/mini-os/lib/sys.c
> > > > +++ b/extras/mini-os/lib/sys.c
> > > > @@ -31,6 +31,7 @@
> > > >  #include <tpm_tis.h>
> > > >  #include <xenbus.h>
> > > >  #include <xenstore.h>
> > > > +#include <poll.h>
> > > >  
> > > >  #include <sys/types.h>
> > > >  #include <sys/unistd.h>
> > > > @@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set
*readfds, fd_set *writefds, fd_set *except
> > > >  #define dump_set(nfds, readfds, writefds, exceptfds,
timeout)
> > > >  #endif
> > > >  
> > > > +#ifdef LIBC_DEBUG
> > > > +static void dump_pollfds(struct pollfd *pfd, int nfds, int
timeout)
> > > > +{
> > > > +    int i, comma, fd;
> > > > +
> > > > +    printk("[");
> > > > +    comma = 0;
> > > > +    for (i = 0; i < nfds; i++) {
> > > > +        fd = pfd[i].fd;
> > > > +        if (comma)
> > > > +            printk(", ");
> > > > +        printk("%d(%c)/%02x", fd,
file_types[files[fd].type],
> > > > +            pfd[i].events);
> > > > +            comma = 1;
> > > > +    }
> > > > +    printk("]");
> > > > +
> > > > +    printk(", %d, %d", nfds, timeout);
> > > > +}
> > > > +#else
> > > > +#define dump_pollfds(pfds, nfds, timeout)
> > > > +#endif
> > > > +
> > > >  /* Just poll without blocking */
> > > >  static int select_poll(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *exceptfds)
> > > >  {
> > > > @@ -983,6 +1007,71 @@ out:
> > > >      return ret;
> > > >  }
> > > >  
> > > > +/* Wrap around select */
> > > > +int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
> > > > +{
> > > > +    int ret;
> > > > +    int i, fd;
> > > > +    struct timeval _timeo, *timeo = NULL;
> > > > +    fd_set rfds, wfds, efds;
> > > > +    int max_fd = -1;
> > > > +
> > > > +    DEBUG("poll(");
> > > > +    dump_pollfds(_pfd, _nfds, _timeout);
> > > > +    DEBUG(")\n");
> > > > +
> > > > +    if (_timeout != -1) {
> > > 
> > > should be _timeout >= 0, any negative value will cause an
infinite wait.
> > > 
> > 
> > Right, I was too obsessed with xenstore code which used -1 to
represent
> > infinity loop.
> > 
> > > > +        /* Timeout in poll is in millisecond. */
> > > > +        _timeo.tv_sec  = _timeout / 1000;
> > > > +        _timeo.tv_usec = (_timeout - _timeo.tv_sec * 1000)
* 1000;
> > > 
> > > why not _timeout % 1000? gcc can also optimize and detect you
have a
> > > division and a module and use only a single instruction in x86.
> > > 
> > 
> > Done.
> > 
> > > > +        timeo = &_timeo;
> > > > +    }
> > > > +
> > > > +    FD_ZERO(&rfds);
> > > > +    FD_ZERO(&wfds);
> > > > +    FD_ZERO(&efds);
> > > > +
> > > > +    for (i = 0; i < _nfds; i++) {
> > > > +        fd = _pfd[i].fd;
> > > 
> > > I think you should check if fd is < 0 and return EINVAL (not
sure about
> > > the error code). Well.. probably you should check fd in range
0..1023.
> > > 
> > 
> > Unfortunately this is not specified in the standard. I guess we can
just
> > ignore print a warning and ignore this fd?
> > 
> >
http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html#tag_16_423
> > 
> 
> Yes, it speaks only of STREAM (I don''t know what''s in
this context).
> 
> I tried what happen on Linux. For values < 0 if just ignore the record.
> Oh... from page you send
> 
> "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()."
> 
Ah, so it is in the description, good.
> For values that are not open if return POLLNVAL in revents.
> 
> Perhaps should be something like
> 
>     ret = 0;
>     for (i = 0; i < _nfds; i++) {
>         fd = _pfd[i].fd;
>         _pfd[i].revents = 0;
>         if (fd < 0) continue;
>         if (fd >= 1024) {
>             _pfd[i].revents = POLLNVAL;
>             ++ret;
>             continue;
>         
I would go for fd >= NOFILE here (NOFILE is the internal limit of
Mini-OS file abstraction).


Wei.

V3 of the patch.

Validate fds before passing it to select. Deal with invalid fds
according to spec.

If fd < 0, revents = 0; if fd >= NOFILE, revents = POLLNVAL.


Wei.

---8<---
From ad953f1e35ecee08b37215ebc9d6196edd744128 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Tue, 19 Feb 2013 12:15:03 +0000
Subject: [PATCH] mini-os: implement poll(2)

It is just a wrapper around select(2).

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 extras/mini-os/include/posix/poll.h |    1 +
 extras/mini-os/lib/sys.c            |  109 ++++++++++++++++++++++++++++++++++-
 2 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 extras/mini-os/include/posix/poll.h

diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
new file mode 100644
index 0000000..06fb41a
--- /dev/null
+++ b/extras/mini-os/include/posix/poll.h
@@ -0,0 +1 @@
+#include <sys/poll.h>
diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
index 3cc3340..46b3933 100644
--- a/extras/mini-os/lib/sys.c
+++ b/extras/mini-os/lib/sys.c
@@ -31,6 +31,7 @@
 #include <tpm_tis.h>
 #include <xenbus.h>
 #include <xenstore.h>
+#include <poll.h>
 
 #include <sys/types.h>
 #include <sys/unistd.h>
@@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *except
 #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
 #endif
 
+#ifdef LIBC_DEBUG
+static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
+{
+    int i, comma, fd;
+
+    printk("[");
+    comma = 0;
+    for (i = 0; i < nfds; i++) {
+        fd = pfd[i].fd;
+        if (comma)
+            printk(", ");
+        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
+            pfd[i].events);
+            comma = 1;
+    }
+    printk("]");
+
+    printk(", %d, %d", nfds, timeout);
+}
+#else
+#define dump_pollfds(pfds, nfds, timeout)
+#endif
+
 /* Just poll without blocking */
 static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set
*exceptfds)
 {
@@ -983,6 +1007,90 @@ out:
     return ret;
 }
 
+/* Wrap around select */
+int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
+{
+    int n;
+    int i, fd;
+    struct timeval _timeo, *timeo = NULL;
+    fd_set rfds, wfds, efds;
+    int max_fd = -1;
+    int saved_errno;
+
+    DEBUG("poll(");
+    dump_pollfds(_pfd, _nfds, _timeout);
+    DEBUG(")\n");
+
+    if (_timeout >= 0) {
+        /* Timeout in poll is in millisecond. */
+        _timeo.tv_sec  = _timeout / 1000;
+        _timeo.tv_usec = (_timeout % 1000) * 1000;
+        timeo = &_timeo;
+    }
+
+    FD_ZERO(&rfds);
+    FD_ZERO(&wfds);
+    FD_ZERO(&efds);
+
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+        _pfd[i].revents = 0;
+
+        /* ignore invalid fds */
+        if (fd < 0 || fd >= NOFILE)
+            continue;
+
+        /* map POLL* into readfds and writefds */
+        /* POLLIN  -> readfds
+         * POLLOUT -> writefds
+         * POLL*   -> none
+         */
+        if (_pfd[i].events & POLLIN)
+            FD_SET(fd, &rfds);
+        if (_pfd[i].events & POLLOUT)
+            FD_SET(fd, &wfds);
+        /* always set exceptfds */
+        FD_SET(fd, &efds);
+        if (fd > max_fd)
+            max_fd = fd;
+    }
+
+    /* don''t care about the return value, but we do need errno */
+    select(max_fd+1, &rfds, &wfds, &efds, timeo);
+
+    n = 0;
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+        if (fd < 0) continue; /* revents = 0 */
+        /* according to spec, we should set revents = POLLNVAL if fd is invalid
*/
+        if (fd >= NOFILE) {
+            n++;
+            _pfd[i].revents |= POLLNVAL;
+            continue;
+        }
+        if (FD_ISSET(fd, &rfds) || FD_ISSET(fd, &wfds) || FD_ISSET(fd,
&efds))
+            n++;
+        if (FD_ISSET(fd, &efds)) {
+            /* anything bad happens we set POLLERR, ignoring PULLHUP
+             * POLLNVAL */
+            _pfd[i].revents |= POLLERR;
+            continue;
+        }
+        if (FD_ISSET(fd, &rfds))
+            _pfd[i].revents |= POLLIN;
+        if (FD_ISSET(fd, &wfds))
+            _pfd[i].revents |= POLLOUT;
+    }
+
+    saved_errno = errno;
+    DEBUG("poll(");
+    dump_pollfds(_pfd, _nfds, _timeout);
+    DEBUG(")\n");
+    errno = saved_errno;
+
+    return n;
+}
+
 #ifdef HAVE_LWIP
 int socket(int domain, int type, int protocol)
 {
@@ -1360,7 +1468,6 @@ unsupported_function(int, tcgetattr, 0);
 unsupported_function(int, grantpt, -1);
 unsupported_function(int, unlockpt, -1);
 unsupported_function(char *, ptsname, NULL);
-unsupported_function(int, poll, -1);
 
 /* net/if.h */
 unsupported_function_log(unsigned int, if_nametoindex, -1);
-- 
1.7.10.4

Wei Liu, le Tue 19 Feb 2013 15:54:13 +0000, a écrit :
> if fd >= NOFILE, revents = POLLNVAL.
"invalid" is also "not opened". You should additionally test
that
files[i].type != FTYPE_NONE.

Samuel

Also, checking for files[i].type should be done on entry to the
function, not after having waited for some time (and perhaps opened some
file descriptor, etc.). I.e.:

Wei Liu, le Tue 19 Feb 2013 15:54:13 +0000, a écrit :
> +        /* ignore invalid fds */
> +        if (fd < 0 || fd >= NOFILE)
> +            continue;
record POLLNVAL as soon as here, and ignore that fd later on.
> +    /* don''t care about the return value, but we do need errno */
> +    select(max_fd+1, &rfds, &wfds, &efds, timeo);
Mmm. I don''t think we don''t care about the return value: if
select
returns -1 with EINTR, poll has to do the same. That''s however probably
the only case we have to take care of.

Samuel

On Tue, 2013-02-19 at 17:39 +0100, Samuel Thibault
wrote:
> Also, checking for files[i].type should be done on entry to the
> function, not after having waited for some time (and perhaps opened some
> file descriptor, etc.). I.e.:
> 
> Wei Liu, le Tue 19 Feb 2013 15:54:13 +0000, a écrit :
> > +        /* ignore invalid fds */
> > +        if (fd < 0 || fd >= NOFILE)
> > +            continue;
> 
> record POLLNVAL as soon as here, and ignore that fd later on.
> 
If you detect the handle is invalid you should not wait in select or set
the timeout to 0. poll should return as soon at it receive an event. In
this case getting POLLNVAL is an event. I don't know if you have to test
all handles in this case or just return the number of fds that got set
to POLLNVAL.
> > +    /* don't care about the return value, but we do need errno */
> > +    select(max_fd+1, &rfds, &wfds, &efds, timeo);
> 
> Mmm. I don't think we don't care about the return value: if select
> returns -1 with EINTR, poll has to do the same. That's however probably
> the only case we have to take care of.
> 
if you get an error you can't consider valid the sets, from Linux man
page (select(2)):

"On error, -1 is returned, and errno is  set  appropriately; the sets
and timeout become undefined, so do not rely on their contents after an
error."
> Samuel
Frediano

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Tue, 2013-02-19 at 17:22 +0000, Frediano Ziglio
wrote:
> On Tue, 2013-02-19 at 17:39 +0100, Samuel Thibault wrote:
> > Also, checking for files[i].type should be done on entry to the
> > function, not after having waited for some time (and perhaps opened
some
> > file descriptor, etc.). I.e.:
> > 
> > Wei Liu, le Tue 19 Feb 2013 15:54:13 +0000, a écrit :
> > > +        /* ignore invalid fds */
> > > +        if (fd < 0 || fd >= NOFILE)
> > > +            continue;
> > 
> > record POLLNVAL as soon as here, and ignore that fd later on.
> > 
> 
> If you detect the handle is invalid you should not wait in select or set
> the timeout to 0. poll should return as soon at it receive an event. In
> this case getting POLLNVAL is an event. I don't know if you have to
test
> all handles in this case or just return the number of fds that got set
> to POLLNVAL.
> 
In Linux's do_poll() implementation, the whole poll array is scanned,
even if some fds in the middle of the loop are not valid.

Wei.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

V4 of the patch

In this version I try to mimic Linux''s do_poll behavior. The pollfd
array is always scanned, even if some fds in the middle of the loops are
invalid. In this case we need to do no-blocking select regardless of
timeout, because we already receive events.

I tried to document the behavior as much as possible, please see
comments in code.


Wei.

----8<---
From 39789af96d9e5a6d4f3d8a9e86546b62918c30df Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Tue, 19 Feb 2013 12:15:03 +0000
Subject: [PATCH] mini-os: implement poll(2)

It is just a wrapper around select(2).

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 extras/mini-os/include/posix/poll.h |    1 +
 extras/mini-os/lib/sys.c            |  117 ++++++++++++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 1 deletion(-)
 create mode 100644 extras/mini-os/include/posix/poll.h

diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
new file mode 100644
index 0000000..06fb41a
--- /dev/null
+++ b/extras/mini-os/include/posix/poll.h
@@ -0,0 +1 @@
+#include <sys/poll.h>
diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
index 3cc3340..10a04eb 100644
--- a/extras/mini-os/lib/sys.c
+++ b/extras/mini-os/lib/sys.c
@@ -31,6 +31,7 @@
 #include <tpm_tis.h>
 #include <xenbus.h>
 #include <xenstore.h>
+#include <poll.h>
 
 #include <sys/types.h>
 #include <sys/unistd.h>
@@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *except
 #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
 #endif
 
+#ifdef LIBC_DEBUG
+static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
+{
+    int i, comma, fd;
+
+    printk("[");
+    comma = 0;
+    for (i = 0; i < nfds; i++) {
+        fd = pfd[i].fd;
+        if (comma)
+            printk(", ");
+        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
+            pfd[i].events);
+            comma = 1;
+    }
+    printk("]");
+
+    printk(", %d, %d", nfds, timeout);
+}
+#else
+#define dump_pollfds(pfds, nfds, timeout)
+#endif
+
 /* Just poll without blocking */
 static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set
*exceptfds)
 {
@@ -983,6 +1007,98 @@ out:
     return ret;
 }
 
+/* Wrap around select */
+int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
+{
+    int n, ret;
+    int i, fd;
+    struct timeval _timeo, *timeo = NULL;
+    fd_set rfds, wfds, efds;
+    int max_fd = -1;
+
+    DEBUG("poll(");
+    dump_pollfds(_pfd, _nfds, _timeout);
+    DEBUG(")\n");
+
+    FD_ZERO(&rfds);
+    FD_ZERO(&wfds);
+    FD_ZERO(&efds);
+
+    n = 0;
+
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+        _pfd[i].revents = 0;
+
+        /* fd < 0, revents = 0, which is already set */
+        if (fd < 0) continue;
+
+        /* fd is invalid, revents = POLLNVAL, increment counter */
+        if (fd >= NOFILE || files[fd].type == FTYPE_NONE) {
+            n++;
+            _pfd[i].revents |= POLLNVAL;
+            continue;
+        }
+
+        /* normal case, map POLL* into readfds and writefds:
+         * POLLIN  -> readfds
+         * POLLOUT -> writefds
+         * POLL*   -> none
+         */
+        if (_pfd[i].events & POLLIN)
+            FD_SET(fd, &rfds);
+        if (_pfd[i].events & POLLOUT)
+            FD_SET(fd, &wfds);
+        /* always set exceptfds */
+        FD_SET(fd, &efds);
+        if (fd > max_fd)
+            max_fd = fd;
+    }
+
+    /* should never sleep when we already have events */
+    if (n) {
+        _timeo.tv_sec  = 0;
+        _timeo.tv_usec = 0;
+        timeo = &_timeo;
+    } else if (_timeout >= 0) {
+        /* normal case, construct _timeout, might sleep */
+        _timeo.tv_sec  = _timeout / 1000;
+        _timeo.tv_usec = (_timeout % 1000) * 1000;
+        timeo = &_timeo;
+    } else {
+        /* _timeout < 0, block forever */
+        timeo = NULL;
+    }
+
+
+    ret = select(max_fd+1, &rfds, &wfds, &efds, timeo);
+    /* error in select, just return, errno is set by select() */
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < _nfds; i++) {
+        fd = _pfd[i].fd;
+
+        /* the revents has already been set for all error case */
+        if (fd < 0 || fd >= NOFILE || files[fd].type == FTYPE_NONE)
+            continue;
+
+        if (FD_ISSET(fd, &rfds) || FD_ISSET(fd, &wfds) || FD_ISSET(fd,
&efds))
+            n++;
+        if (FD_ISSET(fd, &efds)) {
+            /* anything bad happens we set POLLERR */
+            _pfd[i].revents |= POLLERR;
+            continue;
+        }
+        if (FD_ISSET(fd, &rfds))
+            _pfd[i].revents |= POLLIN;
+        if (FD_ISSET(fd, &wfds))
+            _pfd[i].revents |= POLLOUT;
+    }
+
+    return n;
+}
+
 #ifdef HAVE_LWIP
 int socket(int domain, int type, int protocol)
 {
@@ -1360,7 +1476,6 @@ unsupported_function(int, tcgetattr, 0);
 unsupported_function(int, grantpt, -1);
 unsupported_function(int, unlockpt, -1);
 unsupported_function(char *, ptsname, NULL);
-unsupported_function(int, poll, -1);
 
 /* net/if.h */
 unsupported_function_log(unsigned int, if_nametoindex, -1);
-- 
1.7.10.4

Frediano Ziglio, le Tue 19 Feb 2013 17:22:23 +0000, a écrit
:
> On Tue, 2013-02-19 at 17:39 +0100, Samuel Thibault wrote:
> > Also, checking for files[i].type should be done on entry to the
> > function, not after having waited for some time (and perhaps opened
some
> > file descriptor, etc.). I.e.:
> > 
> > Wei Liu, le Tue 19 Feb 2013 15:54:13 +0000, a écrit :
> > > +        /* ignore invalid fds */
> > > +        if (fd < 0 || fd >= NOFILE)
> > > +            continue;
> > 
> > record POLLNVAL as soon as here, and ignore that fd later on.
> > 
> 
> If you detect the handle is invalid you should not wait in select or set
> the timeout to 0. poll should return as soon at it receive an event. In
> this case getting POLLNVAL is an event. I don''t know if you have
to test
> all handles in this case or just return the number of fds that got set
> to POLLNVAL.
I believe you have to test all handles.
> > > +    /* don''t care about the return value, but we do
need errno */
> > > +    select(max_fd+1, &rfds, &wfds, &efds, timeo);
> > 
> > Mmm. I don''t think we don''t care about the return
value: if select
> > returns -1 with EINTR, poll has to do the same. That''s
however probably
> > the only case we have to take care of.
> 
> if you get an error you can''t consider valid the sets, from Linux
man
> page (select(2)):
> 
> "On error, -1 is returned, and errno is  set  appropriately; the sets
> and timeout become undefined, so do not rely on their contents after an
> error."
Yes, sure, but we know what our select() does :)

Samuel

Wei Liu, le Tue 19 Feb 2013 18:36:16 +0000, a écrit :
> V4 of the patch
> 
> In this version I try to mimic Linux''s do_poll behavior. The
pollfd
> array is always scanned, even if some fds in the middle of the loops are
> invalid. In this case we need to do no-blocking select regardless of
> timeout, because we already receive events.
> 
> I tried to document the behavior as much as possible, please see
> comments in code.
> 
> 
> Wei.
> 
> ----8<---
> From 39789af96d9e5a6d4f3d8a9e86546b62918c30df Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Tue, 19 Feb 2013 12:15:03 +0000
> Subject: [PATCH] mini-os: implement poll(2)
> 
> It is just a wrapper around select(2).
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  extras/mini-os/include/posix/poll.h |    1 +
>  extras/mini-os/lib/sys.c            |  117
++++++++++++++++++++++++++++++++++-
>  2 files changed, 117 insertions(+), 1 deletion(-)
>  create mode 100644 extras/mini-os/include/posix/poll.h
> 
> diff --git a/extras/mini-os/include/posix/poll.h
b/extras/mini-os/include/posix/poll.h
> new file mode 100644
> index 0000000..06fb41a
> --- /dev/null
> +++ b/extras/mini-os/include/posix/poll.h
> @@ -0,0 +1 @@
> +#include <sys/poll.h>
> diff --git a/extras/mini-os/lib/sys.c b/extras/mini-os/lib/sys.c
> index 3cc3340..10a04eb 100644
> --- a/extras/mini-os/lib/sys.c
> +++ b/extras/mini-os/lib/sys.c
> @@ -31,6 +31,7 @@
>  #include <tpm_tis.h>
>  #include <xenbus.h>
>  #include <xenstore.h>
> +#include <poll.h>
>  
>  #include <sys/types.h>
>  #include <sys/unistd.h>
> @@ -678,6 +679,29 @@ static void dump_set(int nfds, fd_set *readfds, fd_set
*writefds, fd_set *except
>  #define dump_set(nfds, readfds, writefds, exceptfds, timeout)
>  #endif
>  
> +#ifdef LIBC_DEBUG
> +static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
> +{
> +    int i, comma, fd;
> +
> +    printk("[");
> +    comma = 0;
> +    for (i = 0; i < nfds; i++) {
> +        fd = pfd[i].fd;
> +        if (comma)
> +            printk(", ");
> +        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
> +            pfd[i].events);
> +            comma = 1;
> +    }
> +    printk("]");
> +
> +    printk(", %d, %d", nfds, timeout);
> +}
> +#else
> +#define dump_pollfds(pfds, nfds, timeout)
> +#endif
> +
>  /* Just poll without blocking */
>  static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set
*exceptfds)
>  {
> @@ -983,6 +1007,98 @@ out:
>      return ret;
>  }
>  
> +/* Wrap around select */
> +int poll(struct pollfd _pfd[], nfds_t _nfds, int _timeout)
> +{
> +    int n, ret;
> +    int i, fd;
> +    struct timeval _timeo, *timeo = NULL;
> +    fd_set rfds, wfds, efds;
> +    int max_fd = -1;
> +
> +    DEBUG("poll(");
> +    dump_pollfds(_pfd, _nfds, _timeout);
> +    DEBUG(")\n");
> +
> +    FD_ZERO(&rfds);
> +    FD_ZERO(&wfds);
> +    FD_ZERO(&efds);
> +
> +    n = 0;
> +
> +    for (i = 0; i < _nfds; i++) {
> +        fd = _pfd[i].fd;
> +        _pfd[i].revents = 0;
> +
> +        /* fd < 0, revents = 0, which is already set */
> +        if (fd < 0) continue;
> +
> +        /* fd is invalid, revents = POLLNVAL, increment counter */
> +        if (fd >= NOFILE || files[fd].type == FTYPE_NONE) {
> +            n++;
> +            _pfd[i].revents |= POLLNVAL;
> +            continue;
> +        }
> +
> +        /* normal case, map POLL* into readfds and writefds:
> +         * POLLIN  -> readfds
> +         * POLLOUT -> writefds
> +         * POLL*   -> none
> +         */
> +        if (_pfd[i].events & POLLIN)
> +            FD_SET(fd, &rfds);
> +        if (_pfd[i].events & POLLOUT)
> +            FD_SET(fd, &wfds);
> +        /* always set exceptfds */
> +        FD_SET(fd, &efds);
> +        if (fd > max_fd)
> +            max_fd = fd;
> +    }
> +
> +    /* should never sleep when we already have events */
> +    if (n) {
> +        _timeo.tv_sec  = 0;
> +        _timeo.tv_usec = 0;
> +        timeo = &_timeo;
> +    } else if (_timeout >= 0) {
> +        /* normal case, construct _timeout, might sleep */
> +        _timeo.tv_sec  = _timeout / 1000;
> +        _timeo.tv_usec = (_timeout % 1000) * 1000;
> +        timeo = &_timeo;
> +    } else {
> +        /* _timeout < 0, block forever */
> +        timeo = NULL;
> +    }
> +
> +
> +    ret = select(max_fd+1, &rfds, &wfds, &efds, timeo);
> +    /* error in select, just return, errno is set by select() */
> +    if (ret < 0)
> +        return ret;
> +
> +    for (i = 0; i < _nfds; i++) {
> +        fd = _pfd[i].fd;
> +
> +        /* the revents has already been set for all error case */
> +        if (fd < 0 || fd >= NOFILE || files[fd].type == FTYPE_NONE)
> +            continue;
> +
> +        if (FD_ISSET(fd, &rfds) || FD_ISSET(fd, &wfds) ||
FD_ISSET(fd, &efds))
> +            n++;
> +        if (FD_ISSET(fd, &efds)) {
> +            /* anything bad happens we set POLLERR */
> +            _pfd[i].revents |= POLLERR;
> +            continue;
> +        }
> +        if (FD_ISSET(fd, &rfds))
> +            _pfd[i].revents |= POLLIN;
> +        if (FD_ISSET(fd, &wfds))
> +            _pfd[i].revents |= POLLOUT;
> +    }
> +
> +    return n;
> +}
> +
>  #ifdef HAVE_LWIP
>  int socket(int domain, int type, int protocol)
>  {
> @@ -1360,7 +1476,6 @@ unsupported_function(int, tcgetattr, 0);
>  unsupported_function(int, grantpt, -1);
>  unsupported_function(int, unlockpt, -1);
>  unsupported_function(char *, ptsname, NULL);
> -unsupported_function(int, poll, -1);
>  
>  /* net/if.h */
>  unsupported_function_log(unsigned int, if_nametoindex, -1);
> -- 
> 1.7.10.4
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 
-- 
Samuel
<s> on se croirait en cool : Some browsers close comments on the first
">" character, so to hide script content from such browsers, you
can transpose operands for relational and shift operators (e.g., use "y
< x" rather than "x > y") or use scripting
language-dependent escapes for ">".
 -+- #ens-mim -+-