[PATCH] Add Solaris ports as a tevent backend.

[Jeremy Allison]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-Solaris-ports-as-a-tevent-backend.patch
Type: text/x-diff
Size: 24015 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150212/6aeadbf4/attachment.patch>