You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by st...@apache.org on 2003/02/26 00:25:21 UTC
cvs commit: httpd-2.0/server connection.c
stas 2003/02/25 15:25:20
Modified: server connection.c
Log:
check the return value of ap_run_pre_connection(). So if the
pre_connection phase fails (without setting c->aborted)
ap_run_process_connection is not executed.
Reviewed by: trawick, jim
Revision Changes Path
1.108 +6 -2 httpd-2.0/server/connection.c
Index: connection.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/connection.c,v
retrieving revision 1.107
retrieving revision 1.108
diff -u -r1.107 -r1.108
--- connection.c 3 Feb 2003 17:53:18 -0000 1.107
+++ connection.c 25 Feb 2003 23:25:19 -0000 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;
+ }
+
if (!c->aborted) {
ap_run_process_connection(c);
}
Re: cvs commit: httpd-2.0/server connection.c
Posted by André Malo <nd...@perlig.de>.
* 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, <http://pub.perlig.de/> #
Re: cvs commit: httpd-2.0/server connection.c
Posted by Jeff Trawick <tr...@attglobal.net>.
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
Posted by Stas Bekman <st...@stason.org>.
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 Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org 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
Posted by Jeff Trawick <tr...@attglobal.net>.
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
Posted by Stas Bekman <st...@stason.org>.
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 Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org 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
Posted by Jeff Trawick <tr...@attglobal.net>.
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
Posted by Stas Bekman <st...@stason.org>.
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 Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org 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
Posted by Jeff Trawick <tr...@attglobal.net>.
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
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 03:32 AM 2/26/2003, Greg Stein wrote:
>On Tue, Feb 25, 2003 at 11:25:21PM -0000, stas@apache.org wrote:
>>...
>> +++ connection.c 25 Feb 2003 23:25:19 -0000 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
Posted by Jeff Trawick <tr...@attglobal.net>.
Greg Stein wrote:
> On Tue, Feb 25, 2003 at 11:25:21PM -0000, stas@apache.org 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
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Feb 25, 2003 at 11:25:21PM -0000, stas@apache.org wrote:
>...
> +++ connection.c 25 Feb 2003 23:25:19 -0000 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/