apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@covalent.net>
Subject Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Date Wed, 18 Jul 2001 15:18:25 GMT

-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.

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.  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.  #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 *);


Ryan Bloom                        	rbb@apache.org
Covalent Technologies			rbb@covalent.net

View raw message