httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Bloom <...@covalent.net>
Subject Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Date Sat, 21 Jul 2001 05:37:24 GMT

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

Still a name change, and at the very least this change has nothing to do with 
the fix that you are saying this patch is all about.  This could and should have 
been submitted as a separate patch, because with this change, the patch is 
larger, and harder to review.

If I remember back to my compiler class, I seem to believe that the value should 
have been moved into a register after the first reference, so I don't see a huge 
performance win.  Of course, it has been a while since I took that class.

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

Then you should have left the original code in tact.  At the very least, this should be:

if (wait_for_thread(thd->td, (void *)&thrstat) == B_NO_ERROR) {
    *retval = *thrstat;
}

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

You aren't programming to pthreads right now.  You are programming for APR.
They are different beasts, with different goals and priorities.  In APR, return
values mean something, and just ignoring them is not valid.  If you don't want 
to return a value, then APR_SUCCESS should be returned.

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

I disagree, and I designed this API.  The difference is what you are passing
around.  In pthreads, you are passing in a (void **), which implies that the return
type is unknown.  In APR, it is an integer, and ALL threads exit with a return
code.  It doesn't matter where you know about the code being used.  I happen to
know of multiple closed source programs using APR, neither one of us knows
everything about those programs so making decisions based on who is and who is
not using a function doesn't work.

> > > @@ -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 must be missing something.  retval is an (apr_status_t *), and thrstat
is an (apr_status_t *), but one is going to work properly, and the other
isn't?  I'm sorry, what am I missing?

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

Why do you think the code-test-commit cycle is too long?  This patch has
been out for under one week, which is far shorter than the months that
others have to wait sometimes.  The thing is, what is in the best interest of
the committers, is for patches to be reviewed in detail.  Posting five small patches
that do one thing is a much better way to get patches reviewed.  Shoot, one large
patch that does one thing can be reviewed easily, but this is a large patch that
does many things.

> 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)'??

See above, I still see more problems than you do.  :-)

Ryan

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

Mime
View raw message