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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

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

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