apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Bannert <aa...@ebuilt.com>
Subject Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Date Wed, 18 Jul 2001 15:51:36 GMT
On Wed, Jul 18, 2001 at 08:18:25AM -0700, rbb@covalent.net wrote:
> 
> -1.  Please post patches in-line, because it makes it MUCH easier to reply
> to them and make useful comments.  Also, posting one patch at a time
> would have made this patch easier to follow, and it would have
> allowed me to commit the pieces that I like without having to go in
> and modify the patch itself.  Having said that, my comments follow.

No prob, next time it'll be inline.

> 
> Index: include/apr_thread_proc.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_thread_proc.h,v
> retrieving revision 1.65
> diff -u -r1.65 apr_thread_proc.h
> --- include/apr_thread_proc.h	2001/06/06 18:11:06	1.65
> +++ include/apr_thread_proc.h	2001/07/18 06:27:04
> @@ -125,7 +125,7 @@
>  typedef struct apr_other_child_rec_t  apr_other_child_rec_t;
>  #endif /* APR_HAS_OTHER_CHILD */
> 
> -typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *);
> +typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *,
> apr_thread_t *, apr_pool_t *);
> 
> IMNSHO, this is wrong for two reasons.  #1, if the app has access to the
> apr_thread_t, then they also have access to the pool.

For the app to get access to the apr_thread_t, it must pass it through
in it's own opaque data pointer. This is of course totally unobvious to
the application developer.


>                                                        We have a macro
> called APR_POOL_IMPLEMENT_ACCESSOR that specifically creates accessor
> functions for the pools.  Threads may not implement that function yet, but
> it is far better to allow one method for accessing the pool.

Threads may not implement that function yet because apr_thread_t is
a private os-dependent data structure. Now an application developer
must not only *always* pass the apr_thread_t through it's opaque data
structure (in addition to whatever other data they need to pass), but
once they are done doing that they're going to have to call some
macro to extract the pool from the apr_thread_t? Ignoring the added
complexity and obfuscation from an API user's standpoint just for a
moment, and I still fail to see how this is portable.

[Side note: In order to use APR_POOL_IMPLEMENT_ACCESSOR, will apr_thread_t
have to be a non-os-dependent structure; one that contains the current
os-defined private structure as a member? I see this as an alternative
solution analog to the APR_POOL_IMPLEMENT_ACCESSOR routine, but I reject
the idea that this would be a "good clean API".]

Every other function in APR that would need immediate access to a pool
is passed explicity in the parameter list.


>                                                               #2, why are
> we passing the apr_thread_t as a separate parameter to the thread?  That
> is introducing a layer of indirection that isn't required here.  Just
> create an apr_thread_param_t structure that is:
> 
> struct apr_thread_param_t {
>     apr_thread_t *t;
>     void *data;
> }
> 
> Then, we can pass in this value as the void * to the thread function, and
> be done with it.  This allows us to call the function passed into
> apr_thread_create directly, and it allows us to add more parameters to
> thread creation later if we have to.  I am not saying that we will
> definately have to, but at least this allows us to do so easily.
> 
> Now, we can re-define the APR_THREAD_FUNC api to be
> 
> +typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
> 

Either way is fine for me as long as it's really obvious to the app developer
where to get the apr_thread_t AND the apr_pool_t from. I also agree that
the "unbundled" method you describe here would be easier to maintain in
the case that we'd want to add more parameters later.


So I'm willing to repost the patch (inline this time :) with the
apr_thread_param_t modification suggested above, but I'm not willing
to leave the apr_thread_t as a necessary but missing parameter, nor
am I willing to leave the pool off the parameter list if we can't
get to it easily and portably.

-aaron


Mime
View raw message