You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ryan Bloom <rb...@covalent.net> on 2002/03/07 19:57:40 UTC
RE: Code questions
> server/config.c:396
> return !!(cmd->limited & (AP_METHOD_BIT << methnum));
> ^^
>
> Is that a typo or intentional?
It's intentional. This line always sparks a VERY large debate. The
reason for it is that it is the only way to ensure that you have a 1 or
0 result. By negating twice, the first negation always takes the result
to 0 or 1, and second always gives the opposite result.
> server/core.c:661
> AP_DECLARE(const char *) ap_document_root(request_rec *r) /* Don't
use
> this! */
>
> If we shouldn't use it, why is it still here?
Because people are lazy and most people didn't realize that comment
existed. If nobody is using that function, remove it.
> server/core.c:691
> /* Should probably just get rid of this... the only code that
cares is
> * part of the core anyway (and in fact, it isn't publicised to
other
> * modules).
> */
>
> Read the comment.
Check to make sure nobody uses it, and remove it if nobody does.
> server/listen.c:320
> /*free(lr);*/
>
> Can this go? Was there a future purpose to this call,
> or was it just old code commented out?
It most likely highlights a memory leak more than anything else. These
structures use to be malloc'ed, and free'd at the correct times. Now we
use palloc to allocate them. I would bet that the free was left so that
somebody would remember to look for the leak.
Ryan
Torching ap_response_code_string, WAS: RE: Code questions
Posted by Sander Striker <st...@apache.org>.
> From: Sander Striker [mailto:striker@apache.org]
> Sent: 07 March 2002 20:48
>>> server/core.c:691
>>> /* Should probably just get rid of this... the only code that cares is
>>> * part of the core anyway (and in fact, it isn't publicised to other
>>> * modules).
>>> */
>>>
>>> Read the comment.
>>
>> Check to make sure nobody uses it, and remove it if nobody does.
>
> Ok.
modules/http/http_protocol.c:1923: if ((custom_response = ap_response_code_string(r, idx))) {
modules/http/http_request.c:102: char *custom_response = ap_response_code_string(r, error_index);
Two places where it is used. Suggestions?
Sander
Re: Torching ap_document_root
Posted by Sander van Zoest <sa...@yahoo-inc.com>.
On Sun, Mar 10, 2002 at 04:00:48PM -0800, Greg Stein wrote:
> On Sun, Mar 10, 2002 at 05:26:01PM +0000, Tony Finch wrote:
> > There is no problem with it in principle, but the current
> > implementation is broken. It is too tied to the core's idea
> > of the URI->filesystem mapping and modules are unable to fix
> > that when they know better (mod_vhost_alias, mod_userdir, ...).
> Exactly. DocumentRoot is rather meaningless in the presence of Alias,
> Location, and other directives that define the URL space of the server. The
> DocumentRoot is an arbitrary point. Even worse, it hard-codes the concept of
> the root being tied to the filesystem.
> Relying on that isn't very good. If Module FOO wants to know something about
> the filesystem or the URL space, it should be told explicitly. Or we should
> have a way for files to say "tell me the filesystem path for <this> URL"
> (knowing that it could come back with "doesn't exist"). To some extent, that
> latter question is currently solved by using a sub-request to map a URI to
> the filesystem. Ugly, but that is how the server is currently set up.
We would also need a way for CGIs to know where its websites documents
are. Currently this is usually done with ENV{DOCUMENT_ROOT}, but
as discussed this has its issues. If we would potentially allow
modules to specify where they consider the static documents lie,
then that would solve most of the issues in CGI land.
Then we we can use a URI->FS resolver solution for modules or what
ever seems appropriate.
Cheers,
--
Sander van Zoest +1 (619) 881-3000
Yahoo!, Inc. sander@yahoo-inc.com
<http://www.yahoo.com/> <http://sander.vanzoest.com/>
Re: Torching ap_document_root
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Mar 10, 2002 at 05:26:01PM +0000, Tony Finch wrote:
> On Fri, Mar 08, 2002 at 12:49:16PM -0500, Rodent of Unusual Size wrote:
> >
> > Greg, please take a step back and ask yourself: what *harm*
> > does it do for modules to know the DocumentRoot? If it does no
> > harm, then standing in the way of others just because you can't
> > think of an application *does* do harm.
>
> There is no problem with it in principle, but the current
> implementation is broken. It is too tied to the core's idea
> of the URI->filesystem mapping and modules are unable to fix
> that when they know better (mod_vhost_alias, mod_userdir, ...).
Exactly. DocumentRoot is rather meaningless in the presence of Alias,
Location, and other directives that define the URL space of the server. The
DocumentRoot is an arbitrary point. Even worse, it hard-codes the concept of
the root being tied to the filesystem.
Relying on that isn't very good. If Module FOO wants to know something about
the filesystem or the URL space, it should be told explicitly. Or we should
have a way for files to say "tell me the filesystem path for <this> URL"
(knowing that it could come back with "doesn't exist"). To some extent, that
latter question is currently solved by using a sub-request to map a URI to
the filesystem. Ugly, but that is how the server is currently set up.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Torching ap_document_root
Posted by Tony Finch <do...@dotat.at>.
On Fri, Mar 08, 2002 at 12:49:16PM -0500, Rodent of Unusual Size wrote:
>
> Greg, please take a step back and ask yourself: what *harm*
> does it do for modules to know the DocumentRoot? If it does no
> harm, then standing in the way of others just because you can't
> think of an application *does* do harm.
There is no problem with it in principle, but the current
implementation is broken. It is too tied to the core's idea
of the URI->filesystem mapping and modules are unable to fix
that when they know better (mod_vhost_alias, mod_userdir, ...).
Tony.
Re: Torching ap_document_root
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
Tony Finch wrote:
>
> On Thu, Mar 07, 2002 at 08:23:35PM -0800, Sander van Zoest wrote:
> > On Thu, Mar 07, 2002 at 02:25:56PM -0800, Greg Stein wrote:
> > >
> > > Modules shouldn't be worried about the document root.
>
> This also applies to CGIs. The DOCUMENT_ROOT environment variable is
> a non-standard extension so it should be avoided.
I'm not entirely sure I agree with this. I can think of times
when CGI scripts -- which have access to very little information
unless they're running inside mod_perl -- *would* want to know
the DocumentRoot in order to form proper vhost-specific URIs.
Greg, please take a step back and ask yourself: what *harm*
does it do for modules to know the DocumentRoot? If it does no
harm, then standing in the way of others just because you can't
think of an application *does* do harm.
--
#ken P-)}
Ken Coar, Sanagendamgagwedweinini http://Golux.Com/coar/
Author, developer, opinionist http://Apache-Server.Com/
"Millennium hand and shrimp!"
Re: Torching ap_document_root
Posted by Tony Finch <do...@dotat.at>.
On Thu, Mar 07, 2002 at 08:23:35PM -0800, Sander van Zoest wrote:
> On Thu, Mar 07, 2002 at 02:25:56PM -0800, Greg Stein wrote:
> >
> > Modules shouldn't be worried about the document root. If you need to call
> > this function, then you should ask yourself "why". Modules should be more
> > concerned with r->uri and r->filename.
This also applies to CGIs. The DOCUMENT_ROOT environment variable is
a non-standard extension so it should be avoided.
> In the case of using a CGI with mod_vhost_alias it would be
> nice to be able to have the CGI have access to the virtual document root
> via ENV{DOCUMENT_ROOT}. Right now this doesn't seem possible because
> mod_vhost_alias does not have access to it and otherwise
> ap_add_cgi_vars(r) resets it if we set it via r->subprocess_env in
> mod_vhost_alias.
There are a number of other situations in which the document root setting
is irrelevant, such as normal aliases, mod_userdir, and mod_rewrite;
mod_vhost_alias just makes it more obvious that this is so.
The right fix for CGIs is to allow modules to participate more fully
in setting up environment variables, so that e.g. modules that do
URI->filename mapping can override the DOCUMENT_ROOT setting.
Tony.
--
f.a.n.finch <do...@dotat.at>
FAEROES: SOUTHERLY VEERING WESTERLY, 4 OR 5 INCREASING 6 TO GALE 8, PERHAPS
SEVERE GALE 9 LATER. RAIN THEN WINTRY SHOWERS. MODERATE OR GOOD.
Re: Torching ap_document_root
Posted by Sander van Zoest <sa...@yahoo-inc.com>.
On Thu, Mar 07, 2002 at 02:25:56PM -0800, Greg Stein wrote:
>>>> server/core.c:661
>>>> AP_DECLARE(const char *) ap_document_root(request_rec *r)
> > > > >>> If we shouldn't use it, why is it still here?
> > So the /* don't use this! */ comment should go?
> I would say rewrite it to be something like:
> Modules shouldn't be worried about the document root. If you need to call
> this function, then you should ask yourself "why". Modules should be more
> concerned with r->uri and r->filename.
There are some cases modules need to have access to the document root
(if not modify it for the duration of the request).
In the case of using a CGI with mod_vhost_alias it would be
nice to be able to have the CGI have access to the virtual document root
via ENV{DOCUMENT_ROOT}. Right now this doesn't seem possible because
mod_vhost_alias does not have access to it and otherwise
ap_add_cgi_vars(r) resets it if we set it via r->subprocess_env in
mod_vhost_alias.
We actually had to hack the core to get this to function appropriately.
Cheers,
--
Sander van Zoest +1 (619) 881-3000
Yahoo!, Inc. sander@yahoo-inc.com
<http://www.yahoo.com/> <http://sander.vanzoest.com/>
Re: Torching ap_document_root
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Mar 07, 2002 at 09:23:02PM +0100, Sander Striker wrote:
>...
> > > >>> server/core.c:661
> > > >>> AP_DECLARE(const char *) ap_document_root(request_rec *r) /*
> > Don't
> > > use this! */
> > > >>>
> > > >>> If we shouldn't use it, why is it still here?
>...
> > Having looked at the code now. MO is, yes they are legit. The code
> > reaches into a core private structure to grab the conf->document_root
> > variable. I don't want modules doing that themselves.
>
> So the /* don't use this! */ comment should go?
I would say rewrite it to be something like:
Modules shouldn't be worried about the document root. If you need to call
this function, then you should ask yourself "why". Modules should be more
concerned with r->uri and r->filename.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
RE: Torching ap_document_root, WAS: RE: Code questions
Posted by Sander Striker <st...@apache.org>.
> From: Ryan Bloom [mailto:rbb@covalent.net]
> Sent: 07 March 2002 20:49
> > >>> server/core.c:661
> > >>> AP_DECLARE(const char *) ap_document_root(request_rec *r) /*
> Don't
> > use this! */
> > >>>
> > >>> If we shouldn't use it, why is it still here?
> > >>
> > >> Because people are lazy and most people didn't realize that comment
> > >> existed. If nobody is using that function, remove it.
> > >
> > > Okay, thanks for the heads up.
> >
> > modules/ssl/ssl_engine_vars.c:158: result = (char
> > *)ap_document_root(r);
> > modules/mappers/mod_rewrite.c:1255: if ((ccp =
> > ap_document_root(r)) != NULL) {
> > modules/mappers/mod_rewrite.c:1552: if ((ccp =
> > ap_document_root(r)) != NULL) {
> > modules/mappers/mod_rewrite.c:3492: result =
> ap_document_root(r);
> > server/util_script.c:278: apr_table_addn(e, "DOCUMENT_ROOT",
> > ap_document_root(r)); /* Apache */
> >
> > Ofcourse there are always places where such a function is used...
> > Question is now, are they legit? Should they be changed?
>
> Having looked at the code now. MO is, yes they are legit. The code
> reaches into a core private structure to grab the conf->document_root
> variable. I don't want modules doing that themselves.
So the /* don't use this! */ comment should go?
> Ryan
Sander
RE: Torching ap_document_root, WAS: RE: Code questions
Posted by Ryan Bloom <rb...@covalent.net>.
> >>> server/core.c:661
> >>> AP_DECLARE(const char *) ap_document_root(request_rec *r) /*
Don't
> use this! */
> >>>
> >>> If we shouldn't use it, why is it still here?
> >>
> >> Because people are lazy and most people didn't realize that comment
> >> existed. If nobody is using that function, remove it.
> >
> > Okay, thanks for the heads up.
>
> modules/ssl/ssl_engine_vars.c:158: result = (char
> *)ap_document_root(r);
> modules/mappers/mod_rewrite.c:1255: if ((ccp =
> ap_document_root(r)) != NULL) {
> modules/mappers/mod_rewrite.c:1552: if ((ccp =
> ap_document_root(r)) != NULL) {
> modules/mappers/mod_rewrite.c:3492: result =
ap_document_root(r);
> server/util_script.c:278: apr_table_addn(e, "DOCUMENT_ROOT",
> ap_document_root(r)); /* Apache */
>
> Ofcourse there are always places where such a function is used...
> Question is now, are they legit? Should they be changed?
Having looked at the code now. MO is, yes they are legit. The code
reaches into a core private structure to grab the conf->document_root
variable. I don't want modules doing that themselves.
Ryan
Torching ap_document_root, WAS: RE: Code questions
Posted by Sander Striker <st...@apache.org>.
> From: Sander Striker [mailto:striker@apache.org]
> Sent: 07 March 2002 20:48
>>> server/core.c:661
>>> AP_DECLARE(const char *) ap_document_root(request_rec *r) /* Don't use this! */
>>>
>>> If we shouldn't use it, why is it still here?
>>
>> Because people are lazy and most people didn't realize that comment
>> existed. If nobody is using that function, remove it.
>
> Okay, thanks for the heads up.
modules/ssl/ssl_engine_vars.c:158: result = (char *)ap_document_root(r);
modules/mappers/mod_rewrite.c:1255: if ((ccp = ap_document_root(r)) != NULL) {
modules/mappers/mod_rewrite.c:1552: if ((ccp = ap_document_root(r)) != NULL) {
modules/mappers/mod_rewrite.c:3492: result = ap_document_root(r);
server/util_script.c:278: apr_table_addn(e, "DOCUMENT_ROOT", ap_document_root(r)); /* Apache */
Ofcourse there are always places where such a function is used...
Question is now, are they legit? Should they be changed?
Sander
RE: Code questions
Posted by Sander Striker <st...@apache.org>.
> From: Ryan Bloom [mailto:rbb@covalent.net]
> Sent: 07 March 2002 19:58
>> server/config.c:396
>> return !!(cmd->limited & (AP_METHOD_BIT << methnum));
>> ^^
>>
>> Is that a typo or intentional?
>
> It's intentional. This line always sparks a VERY large debate.
Then why didn't any one leave a nice comment behind so that this
doesn't happen again? ;)
> The reason for it is that it is the only way to ensure that you have a 1 or
> 0 result. By negating twice, the first negation always takes the result
> to 0 or 1, and second always gives the opposite result.
It's not exactly the only way. There are two more:
1) return (cmd->limited & (AP_METHOD_BIT << methnum)) ? 1 : 0;
2) return (cmd->limited & (AP_METHOD_BIT << methnum)) != 0;
And method 3, this entire block:
/*
* A method number either hardcoded into apache or
* added by a module and registered.
*/
if (methnum != M_INVALID) {
return (cmd->limited & (AP_METHOD_BIT << methnum)) ? 1 : 0;
}
return 0; /* not found */
can be written as:
/*
* A method number either hardcoded into apache or
* added by a module and registered.
*/
return (methnum != M_INVALID
&& (cmd->limited & (AP_METHOD_BIT << methnum)));
If noone steps up I'll change it to method 1, since that is
most clear to read for everyone I think.
>> server/core.c:661
>> AP_DECLARE(const char *) ap_document_root(request_rec *r) /* Don't use this! */
>>
>> If we shouldn't use it, why is it still here?
>
> Because people are lazy and most people didn't realize that comment
> existed. If nobody is using that function, remove it.
Okay, thanks for the heads up.
>> server/core.c:691
>> /* Should probably just get rid of this... the only code that cares is
>> * part of the core anyway (and in fact, it isn't publicised to other
>> * modules).
>> */
>>
>> Read the comment.
>
> Check to make sure nobody uses it, and remove it if nobody does.
Ok.
>> server/listen.c:320
>> /*free(lr);*/
>>
>> Can this go? Was there a future purpose to this call,
>> or was it just old code commented out?
>
> It most likely highlights a memory leak more than anything else. These
> structures use to be malloc'ed, and free'd at the correct times. Now we
> use palloc to allocate them. I would bet that the free was left so that
> somebody would remember to look for the leak.
Consider the line torched.
> Ryan
Thanks,
Sander