[PATCH] Fix epoll backend to allow separate read/write events on one fd.

[Jeremy Allison]

On Thu, Feb 14, 2013 at 03:18:27PM +0100, Stefan (metze) Metzmacher wrote:
> Hi Jeremy,
> 
> >> The tdgram_context and tstream_context abstractions only need one tevent_fd.
> >>
> >> At least we should allow only one TEVENT_FD_READ handler and only one
> >> TEVENT_FD_WRITE handler
> >> for each fd.
> > 
> > That is the idiom I think it's right to standardize on,
> > and that's what my patch does.
> 
> I see the protection against more than two handlers.
> 
> >> But I'd think it's better to avoid additional complexity in
> >> the epoll backend,
> >> which is already complex.
> > 
> > Yeah yeah - it's still a *lot* simpler than the poll backend,
> > even *with* my multiplexing patch :-) :-).
> 
> Ok, looking at the patches your're right :-)
> 
> The tricky part is the EPOLL_ADDITIONAL_FD_FLAG_GOT_ERROR
> and EPOLL_ADDITIONAL_FD_FLAG_REPORT_ERROR. I don't think that's
> handled correct in your patches.

Ok, I'll take a look and see what I might have missed.

> I'm also not sure if the EPOLL_ADDITIONAL_FD_FLAG_HAS_EVENT is filled
> and cleared correctly.

I was pretty sure I got that right, but will examine
more closely in case I missed something.

> Can you change the code to store the pointer to the other event
> in fde->addtitional_data toghether with the
> EPOLL_ADDITIONAL_FD_WAS_MULTIPLEXED
> flag? That would avoid a list traverse for a lot of cases.
> We need a destructor that removes the cross-reference on delete.

Ok, I'll look at this.

> This way we can preferr the write handler over the read handler
> and also make sure that tevent_fd_set_flags doesn't set the same flag
> to both handlers.

That's a change in behavior of the epoll backend. I'm not
disagreeing, but I think that should be a subsequent patch,
not part of this one.

> It would be good to enforce the rule of just one fd event for
> TEVENT_FD_READ and just one fd event for TEVENT_FD_WRITE.
> We need to enforce this on tevent_add_fd() and on tevent_fd_set_flags()
> and for all backends.

Again, I think that should be a follow-up patch, not
part of this. Once we've got this fixed in the epoll backend
then a subsequent patch should enforce these rules.

> It's good to get this finally fixed, thanks!

Thanks for the review !

Cheers,

	Jeremy.