apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robert Norris <...@cataclysm.cx>
Subject Re: RESEND: PATCH: Update requested events for a descriptor that is already in a poll set
Date Mon, 15 Mar 2004 21:39:27 GMT
On Mon, Mar 15, 2004 at 10:11:09AM -0800, rbb@rkbloom.net wrote:
> 1)  This patch isn't complete.  At the very least we need a patch to the header
> that includes doxygen comments that document this new API.  Preferably, a new
> patch would also include a patch to testpoll to test this feature.  It should
> also include a stub function in all non-unix poll files, so that other builds
> don't fail.

The Doxygen bits are there. I haven't done the test or the stubs a)
because I haven't dug deeply enough to figure out exactly how to do this
and b) because I'm not actually sure if this is the "right" way to do
this.

Rob.

> > > The optimisation that I'd like to do is to maintain a pointer (by index
> > > or explicit) for each descriptor that points to the descriptors'
> > > underlying pollfd structure, and just use this. This would lose the
> > > ability to enter a socket into the set multiple times with different
> > > events, as is currently possible, but would eliminate the search.
> 
> I'm not sure that ability really exists currently (instead of it just being an
> accident of implementation), and if it does I am positive that it isn't
> portable.  Consider the simple case where the underlying system is using select
> instead() of poll().  When using select, you can't have the same descriptor in
> the pollset twice, the system doesn't support it.

Agreed.

> > > The only problem with this is that such a pointer would really belong in
> > > apr_pollfd_t, except that its specific to the poll() implementation, and
> > > that type is not opaque. So I'd appreciate some pointers on how to best
> > > do this.
> 
> The easiest way is to create a union of incomplete types that actually implement
> the pointer.  This is how pollfd_t has a pointer to the apr_file_t and
> apr_socket_t structures.  That will make the code portable at least, and users
> outside of APR won't be able to mess up the pointer.

Understood.

> In general, I like the idea for this API, but I would much rather that it was
> implemented as:
> 
> apr_status_t apr_pollset_update(....) {
>     apr_pollset_remove(...);
>     apr_pollset_add(...);
> }
> 
> This would remove the possible bugs from forgetting to call things like FD_CLR.
>  It would also mean that adding this function to all poll implementations would
> be trivial.  With the index pointer added, it also wouldn't be much less
> performant than the original version.

You're probably right. I'll see about getting an updated patch that
incorporates these changes.

Thanks for the feedback!

Rob.

-- 
Robert Norris                                       GPG: 1024D/FC18E6C2
Email+Jabber: rob@cataclysm.cx                Web: http://cataclysm.cx/

Mime
View raw message