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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

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

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