[PATCH] Fix epoll backend to allow separate read/write events on one fd.
jra at samba.org
Thu Feb 14 11:04:58 MST 2013
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
> 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 !
More information about the samba-technical