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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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.


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