On Mon, Mar 15, 2004 at 10:11:09AM -0800, [EMAIL PROTECTED] 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: [EMAIL PROTECTED] Web: http://cataclysm.cx/
signature.asc
Description: Digital signature
