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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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