[PATCH] Add Solaris ports as a tevent backend.

[Ira Cooper]

What happens if someone has epoll + Solaris ports?  I know that the Illumos
folks have been implementing various Linux syscalls...  (You have a #ifdef
that needs checking at least...)

I'm going to say, you are going against part of the goal of a ports
backend, by traversing all the registered events.  The goal is not to have
performance scale with the number of registered events.  (Like the poll
backend.)

You might consider a doubly linked list, and then I think the whole
"associated event" concept goes away, and you end up with 2 lists, which is
how it looks like it wants to be...

As is... it feels awkward.

My $0.02,

-Ira

On Thu, Feb 12, 2015 at 2:59 PM, Jeremy Allison <jra at samba.org> wrote:

> On Thu, Feb 12, 2015 at 07:05:32PM +0100, David Disseldorp wrote:
> > Hi Jeremy,
> >
> > The patch looks good, but I unfortunately don't have a Solaris system to
> > test against.
>
> That's why I resent after Thomas Schulz had been running it
> in production for many months :-).
>
> > A few comments below, which I'd like to see addressed
> > before pushing...
> >
> > On Tue, 10 Feb 2015 14:59:50 -0800, Jeremy Allison wrote:
> >
> > ...
> > > --- a/lib/replace/system/select.h
> > > +++ b/lib/replace/system/select.h
> > > @@ -34,6 +34,10 @@
> > >  #include <sys/epoll.h>
> > >  #endif
> > >
> > > +#ifdef HAVE_PORT_H
> > > +#include <port.h>
> > > +#endif
> >
> > Should probably use HAVE_SOLARIS_PORTS here.
>
> Fixed.
>
> > > @@ -126,6 +126,10 @@ static void tevent_backend_init(void)
> > >  #ifdef HAVE_EPOLL
> > >     tevent_epoll_init();
> > >  #endif
> > > +#ifdef HAVE_SOLARIS_PORTS
> > > +   tevent_port_init();
> > > +#endif
> > > +
> > >     tevent_standard_init();
> > >  }
> > >
> > > @@ -139,7 +143,11 @@ _PRIVATE_ const struct tevent_ops
> *tevent_find_ops_byname(const char *name)
> > >             name = tevent_default_backend;
> > >     }
> > >     if (name == NULL) {
> > > +#ifdef HAVE_SOLARIS_PORTS
> > > +           name = "port";
> > > +#else
> > >             name = "standard";
> > > +#endif
> >
> > tevent_port_init() could call tevent_set_default_backend() to avoid this
> > hunk.
>
> Done.
>
> > > --- /dev/null
> > > +++ b/lib/tevent/tevent_port.c
> > > @@ -0,0 +1,767 @@
> >
> > ...
> >
> > > +static bool store_port_association(struct port_event_context *port_ev,
> > > +                           struct tevent_fd *fde,
> > > +                           int events)
> > > +{
> > > +   struct port_associate_vals *val;
> > > +
> > > +   for (val = port_ev->po_vals; val; val = val->next) {
> > > +           if (val->fde->fd == fde->fd) {
> > > +                   /* Association already attached to fd. */
> > > +                   if (val->events != events) {
> > > +                           val->events = events;
> > > +                           val->associated_event = false;
> > > +                   }
> > > +                   return true;
> > > +           }
> > > +   }
> > > +
> > > +   val = talloc_zero(port_ev, struct port_associate_vals);
> > > +   if (val == NULL) {
> > > +           return false;
> > > +   }
> > > +
> > > +   val->port_ev = port_ev;
> > > +   val->fde = fde;
> > > +   val->events = events;
> > > +   val->associated_event = false;
> > > +
> > > +   DLIST_ADD(port_ev->po_vals, val);
> > > +   talloc_set_destructor(val, port_associate_vals_destructor);
> > > +
> > > +   return true;
> > > +}
> >
> > As the port_association is per-fde, wouldn't it be simpler to store it
> > directly in fde->additional_data, alongside any multiplexed-fde? That
> > way you'd avoid the lookup on store and delete, and would only need
> > to walk the ev->fd_events list in associate_all_events().
>
> Can I take this one as a request for enhancement once it's
> gone in ? I'm loath to make such sweeping changes initially
> as it's been running well under production for many months.
>
> Once it's in the tree we can optimize after if required. Right
> now the logic is really close to the epoll backend, which makes
> it easier to understand (for me at least :-).
>
> > > +static struct tevent_fd *port_event_add_fd(struct tevent_context *ev,
> TALLOC_CTX *mem_ctx,
> > > +                                       int fd, uint16_t flags,
> > > +                                       tevent_fd_handler_t handler,
> > > +                                       void *private_data,
> > > +                                       const char *handler_name,
> > > +                                       const char *location)
> > > +{
> > > +   struct port_event_context *port_ev =
> talloc_get_type(ev->additional_data,
> > > +                                                      struct
> port_event_context);
> >
> > tevent_epoll was recently changed to use talloc_get_type_abort() here,
> > please do the same.
>
> Done.
>
> > > +static void port_event_set_fd_flags(struct tevent_fd *fde, uint16_t
> flags)
> > > +{
> > > +   struct tevent_context *ev;
> > > +   struct port_event_context *port_ev;
> > > +
> > > +   if (fde->flags == flags) {
> > > +           return;
> > > +   }
> > > +
> > > +   ev = fde->event_ctx;
> > > +   port_ev = talloc_get_type(ev->additional_data, struct
> port_event_context);
> >
> > talloc_get_type_abort() here too.
>
> Done.
>
> Updated version attached.
>
> Thanks for the review !
>
> Jeremy.
>