httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Bannert <aa...@ebuilt.com>
Subject Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Date Sat, 21 Jul 2001 04:57:09 GMT
On Fri, Jul 20, 2001 at 09:20:49PM -0700, Ryan Bloom wrote:
> On Friday 20 July 2001 10:30, Aaron Bannert wrote:
> > I submitted most of this patch last weekend. Since then I've fixed
> > a small bug in the apr_thread_exit() patch I made (I could really
> > use APR commit access for the little things like this :). I haven't
> > heard any objections (Ryan ;), so I assume these changes were
> > acceptable.
> 
> Grrrrrr......  This patch does all sorts of stuff that it doesn't need to do.  :-(  The
BeOS code was changed drastically, although most of the changes are more cosmetic than anything
else, some are more serious.  I have decided not to apply this patch because some of the changes
are just not necessary, and others are just wrong given APR's general coding style.  I have
more comments in-line.
> 
> > Index: srclib/apr/threadproc/beos/thread.c
> > ===================================================================
> > RCS file: /home/cvspublic/apr/threadproc/beos/thread.c,v
> > retrieving revision 1.22
> > diff -u -r1.22 thread.c
> > --- srclib/apr/threadproc/beos/thread.c	2001/06/14 18:52:05	1.22
> > +++ srclib/apr/threadproc/beos/thread.c	2001/07/20 17:11:33
> > @@ -94,13 +94,17 @@
> >  {
> >      int32 temp;
> >      apr_status_t stat;
> > +    struct thread_info_t *thr_info;
> 
> What is this variable for?  It's never used anywhere.

That was a mistake. I removed it in the unix tree, but missed in beos
(as I don't have beos).


> 
> > +    apr_thread_t *thread;
> > +    apr_thread_param_t *thrparm;
> >
> > -    (*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
> > -    if ((*new) == NULL) {
> > +    thread = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t)); 
> > +    (*new) = thread;
> 
> Why did we add a new variable here?  A BIG portion of this patch could
> have been avoided, making it easier to review, if variable name changes
> like this hadn't been included.

Readability, consistency, and even a slight performance boost.

I think os2 had it right, so I changed the others to be the same. Not
really a name change at all.


> >  apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> > *thd) {
> > -    if (wait_for_thread(thd->td,(void *)&retval) == B_NO_ERROR) {
> > +    apr_status_t *thrstat = NULL;
> > +
> > +    if (wait_for_thread(thd->td,(void *)&thrstat) == B_NO_ERROR) {
> > +        if (retval != NULL && thrstat != NULL)
> > +            *retval = *thrstat;
> 
> What is this change about?  If the user passed in NULL for retval, then we
> should just seg fault here.  Why was this all added?

This change mirrors the same change to unix. Again, because I don't have
access to beos I can only do my best to get that code as close as possible
to what I think it should be, but in the end I'd need someone who is
familiar with that system to review my changes.

> > -    *retval = (apr_status_t)thd->rv;
> > +    if (retval != NULL)
> > +        *retval = (apr_status_t)thd->rv;
> 
> Again, we don't need this change.

How is NULL an invalid return value from a thread? Here are the API docs:

/**
 * block until the desired thread stops executing.
 * @param retval The return value from the dead thread.
 * @param thd The thread to join
 * @deffunc apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd);
 */

What if you don't want to pass it an apr_status_t, you just want to join?
My changes bring apr_thread_join() more inline with pthread_join().
It's perfectly valid to give pthread_join() a NULL and it won't bother
trying to set the return value for you.


This particular function is not in use *anywhere* in the httpd-2.0 or
apr/apr-util code trees. AFAIK, flood is the only place where this is
being used. I would be very concerned about changing functionality of
existing code if that were not the case, but since it is not I think
there is MUCH room for interpretation.


> > Index: srclib/apr/threadproc/unix/thread.c
> > ===================================================================
> > RCS file: /home/cvspublic/apr/threadproc/unix/thread.c,v
> > retrieving revision 1.39
> > diff -u -r1.39 thread.c
> > --- srclib/apr/threadproc/unix/thread.c	2001/06/14 18:52:09	1.39
> > +++ srclib/apr/threadproc/unix/thread.c	2001/07/20 17:11:33
> > @@ -122,32 +122,43 @@
> >  {
> >      apr_status_t stat;
> >      pthread_attr_t *temp;
> > +    apr_thread_t *thread;
> > +    apr_thread_param_t *thrparm;
> >
> > -    (*new) = (apr_thread_t *)apr_pcalloc(cont,
> > sizeof(apr_thread_t)); +    thread = (apr_thread_t
> > *)apr_pcalloc(cont, sizeof(apr_thread_t)); +    (*new) = thread;
> >
> > -    if ((*new) == NULL) {
> > +    if (thread == NULL) {
> 
> Same as above.  Changing variable names is not a good thing to
> do in the middle of a patch this size.

Same reason as before. Changing implementations to be consistent
improves readability. It also reduces the number of indirections.


> > @@ -178,8 +189,11 @@
> >  apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> > *thd) {
> >      apr_status_t stat;
> > +    apr_status_t *thrstat = NULL;
> >
> > -    if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0) {
> > +    if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0) {
> > +        if (retval != NULL && thrstat != NULL)
> > +            *retval = *thrstat;
> 
> And again, this could have been fixed by just adding a * to the (void *), couldn't it?

The cast just gets rid of the warning, but doesn't change the value of
the parameter. pthread_join() will set thrstat to point to the value
returned from apr_thread_exit(), but that is just a pass-by-value parameter
so it never makes it back out to the caller. That is why it didn't work
before.


> I have skipped the Windows section of the patch, but it is more of the same.
> Fix those comments, and I will review the patch again.  MNSO is that the goal of
> patches should be to change as little code as possible.  The more changes there are,
> the harder it is to review the patch.  If a patch is generated, and there isn't a need
for
> a line of code to have changed, the person who created the patch should go through
> and ask why that line was changed.

Your inspection is valid, but I'm not sure it is in the best interest of
the commiters on this list for me to be supplying many smaller and less
important patches all day long. The code-test-commit cycle is just way
to long for that. In any case, I will try to keep the patches short and
sweet in the future.

As for updating this patch, the only problem that I see is there was a
variable left over that is no longer is use, and it's declaration should
be removed. Do you still want me to revert all the 'thread' identifiers
back to '(*new)'??

-aaron



Mime
View raw message