Re: cvs commit: httpd-2.0/server connection.c

[André Malo]

* Jeff Trawick wrote:

> if it changes code from prior release and it is user visible, log it;
> otherwise, probably not if you ask me unless it is something really big
> like making mod_rewrite easier to read :)

hehehe ;-) Actually a lot of things in mod_rewrite can be simplified and 
implemented more efficient and I plan to do that sometimes (perhaps in a 
step by step procedure). The current changes are intended to be 
backportable bugfixes. But when they're done ...

nd
-- 
die (eval q-qq:Just Another Perl Hacker
:-)

# André Malo,  #

Re: cvs commit: httpd-2.0/server connection.c

[Jeff Trawick]

Stas Bekman wrote:

> Jeff Trawick wrote:
>
> > Stas Bekman wrote:
> >
> >> Should I update Changes for this fix as well?
> >
>
> I was asking whether I should have also logged this last change:
>
>AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c, void *csd)
>{
>   -apr_status_t rc;
>   +int rc;
>
> My common sense says no, but one can't be too sure...
oh, right you are...  don't log something like that

if it changes code from prior release and it is user visible, log it;
otherwise, probably not if you ask me unless it is something really big 
like making mod_rewrite easier to read :)

Re: cvs commit: httpd-2.0/server connection.c

[Stas Bekman]

Jeff Trawick wrote:
Stas Bekman wrote:

Should I update Changes for this fix as well?


personally I would have put something like this in changes:

*) Fix a segfault when a pre_connection hook fails the connection.
   [name]
That has been added already.

I was asking whether I should have also logged this last change:

   AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c, void *csd)
   {
  -apr_status_t rc;
  +int rc;
My common sense says no, but one can't be too sure...

__
Stas BekmanJAm_pH --> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

Re: cvs commit: httpd-2.0/server connection.c

[Jeff Trawick]

Stas Bekman wrote:

Should I update Changes for this fix as well?
personally I would have put something like this in changes:

*) Fix a segfault when a pre_connection hook fails the connection.
   [name]

Re: cvs commit: httpd-2.0/server connection.c

[Stas Bekman]

Jeff Trawick wrote:
Stas Bekman wrote:

So, everybody agrees that it should be:

-apr_status_t rc;
+int rc;
correct?


yes!
Thanks you Jeff. I've committed the fix.

Should I update Changes for this fix as well?

__
Stas BekmanJAm_pH --> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

Re: cvs commit: httpd-2.0/server connection.c

[Jeff Trawick]

Stas Bekman wrote:

So, everybody agrees that it should be:

-apr_status_t rc;
+int rc;
correct?
yes!

Re: cvs commit: httpd-2.0/server connection.c

[Stas Bekman]

Jeff Trawick wrote:
William A. Rowe, Jr. wrote:

They should, ap_run_pre_connection is an Apache hook.  Yes, it returns
an int, so the only change here should be
>>  -apr_status_t rc;
>>  +int rc;
We aren't calling apr_ function here, and hooks always allow OK, DONE,
or (result).


ahh, that's the missing piece of commentary on Stas' commit to explain 
various confusion :)
So, everybody agrees that it should be:

-apr_status_t rc;
+int rc;
correct?

__
Stas BekmanJAm_pH --> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

Re: cvs commit: httpd-2.0/server connection.c

[Jeff Trawick]

William A. Rowe, Jr. wrote:

They should, ap_run_pre_connection is an Apache hook.  Yes, it returns
an int, so the only change here should be
>>  -apr_status_t rc;
>>  +int rc;
We aren't calling apr_ function here, and hooks always allow OK, DONE,
or (result).
ahh, that's the missing piece of commentary on Stas' commit to explain 
various confusion :)

Re: cvs commit: httpd-2.0/server connection.c

[William A. Rowe, Jr.]

At 03:32 AM 2/26/2003, Greg Stein wrote:
>On Tue, Feb 25, 2003 at 11:25:21PM -, [EMAIL PROTECTED] wrote:
>>...
>>   +++ connection.c25 Feb 2003 23:25:19 -  1.108
>>   @@ -199,10 +199,14 @@
>>
>>AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c, void *csd)
>>{
>>   +apr_status_t rc;
>>ap_update_vhost_given_ip(c);
>>
>>   -ap_run_pre_connection(c, csd);
>>   -
>>   +rc = ap_run_pre_connection(c, csd);
>>   +if (rc != OK && rc != DONE) {
>>   +c->aborted = 1;
>>   +}
>
>OK and DONE are not apr_status_t values. If you're truly returning a status,
>then you simply check for non-zero (or != APR_SUCCESS). If you truly want to
>return OK/DONE types of values, then the type of rc should be "int".
>
>IMO, since you aren't even at HTTP processing at this point, it *should* be
>an apr_status_t, and the OK/DONE types of values don't enter the picture.

They should, ap_run_pre_connection is an Apache hook.  Yes, it returns
an int, so the only change here should be

>>   -apr_status_t rc;
>>   +int rc;

We aren't calling apr_ function here, and hooks always allow OK, DONE,
or (result).

Bill 

Re: cvs commit: httpd-2.0/server connection.c

[Jeff Trawick]

Greg Stein wrote:

On Tue, Feb 25, 2003 at 11:25:21PM -, [EMAIL PROTECTED] wrote:

OK and DONE are not apr_status_t values. If you're truly returning a 
status,
then you simply check for non-zero (or != APR_SUCCESS). If you truly 
want to
return OK/DONE types of values, then the type of rc should be "int".

IMO, since you aren't even at HTTP processing at this point, it 
*should* be
an apr_status_t, and the OK/DONE types of values don't enter the picture.
Just do:


Greg, are you suggesting that Stas change the API for 2.1?  I can't tell 
whether

[a] you don't like the way he implemented the check
[b] you think the original pre_connection declarations were wrong and 
you want him to both change the API as well as implement the error 
checking that he found necessary
[c] something else

Re: cvs commit: httpd-2.0/server connection.c

[Greg Stein]

On Tue, Feb 25, 2003 at 11:25:21PM -, [EMAIL PROTECTED] wrote:
>...
>   +++ connection.c25 Feb 2003 23:25:19 -  1.108
>   @@ -199,10 +199,14 @@
>
>AP_CORE_DECLARE(void) ap_process_connection(conn_rec *c, void *csd)
>{
>   +apr_status_t rc;
>ap_update_vhost_given_ip(c);
>
>   -ap_run_pre_connection(c, csd);
>   -
>   +rc = ap_run_pre_connection(c, csd);
>   +if (rc != OK && rc != DONE) {
>   +c->aborted = 1;
>   +}

OK and DONE are not apr_status_t values. If you're truly returning a status,
then you simply check for non-zero (or != APR_SUCCESS). If you truly want to
return OK/DONE types of values, then the type of rc should be "int".

IMO, since you aren't even at HTTP processing at this point, it *should* be
an apr_status_t, and the OK/DONE types of values don't enter the picture.
Just do:

  if (ap_run_pre_connection(c, csd) != APR_SUCCESS)
  c->aborted = 1;

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/