Re: Code questions (server/protocol.c)
On Mon, Mar 11, 2002 at 03:03:02PM +0100, Sander Striker wrote:
Hi,
server/protocol.c:136
if (ap_strcasestr(type, charset=) != NULL) {
/* already has parameter, do nothing */
/* XXX we don't check the validity */
;
}
Validity checking seems like a good idea, someone
want to grab this one?
Seems like you volunteered. =)
server/protocol.c:658
#if 0
/* XXX If we want to keep track of the Method, the protocol module should do
* it. That support isn't in the scoreboard yet. Hopefully next week
* sometime. rbb */
ap_update_connection_status(AP_CHILD_THREAD_FROM_ID(conn-id), Method,
r-method);
#endif
Can this block go? 'next week' has been over 6 months now :)
Ah, toss it. I don't think the scoreboard needs to record the
method.
server/protocol.c:823
r-request_config = ap_create_request_config(r-pool);
/* Must be set before we run create request hook */
r-proto_output_filters = conn-output_filters;
r-output_filters = r-proto_output_filters;
r-proto_input_filters = conn-input_filters;
r-input_filters = r-proto_input_filters;
ap_run_create_request(r);
To what code does the comment refer? The line above it, or
the block under it?
The line below. The filters must be set before we run that hook.
server/protocol.c:1133
/* Humm, is this check the best it can be?
* - protocol = HTTP/1.1 implies support for chunking
* - non-keepalive implies the end of byte stream will be signaled
*by a connection close
* In both cases, we can send bytes to the client w/o needing to
* compute content-length.
* Todo:
* We should be able to force connection close from this filter
* when we see we are buffering too much.
*/
The comment says it all.
Um, I think the conditional is right. The todo may be wrong as I
think we could just send flush down, but I'm not really clear
what its context is.
server/protocol.c:1290
AP_DECLARE(size_t) ap_send_mmap(apr_mmap_t *mm, request_rec *r, size_t offset,
size_t length)
I reckon the size_t's are left here intentional, weren't forgotten when
switching to apr_size_t?
Does anything use this? I can't fathom any situation where this should
be called - this screams an MMAP bucket to be passed down. size_t
should refer to anything in memory, while off_t refers to position in
a file. We have some mismatches in that. If anyone would like a
nice project, cleaning up our bucket/brigade types to remove the
dependency on apr_size_t/apr_off_t would be real nice before GA
(create apr_bucket_size_t and apr_brigade_size_t). But, it'll be
painful.
server/protocol.c:1338
/* future optimization: record some flags in the request_rec to
* say whether we've added our filter, and whether it is first.
*/
Still valid?
I think so. Boy, this looks like it could get real expensive.
Ouch.
server/protocol.c:1501
/* ### TODO: if the total output is large, put all the strings
* ### into a single brigade, rather than flushing each time we
* ### fill the buffer
*/
And that's another one for our performance freaks ;)
Definitely. That todo seems the right thing to do
here. -- justin
RE: Code questions (server/protocol.c)
server/protocol.c:1290 AP_DECLARE(size_t) ap_send_mmap(apr_mmap_t *mm, request_rec *r, size_t offset, size_t length) I reckon the size_t's are left here intentional, weren't forgotten when switching to apr_size_t? Does anything use this? I can't fathom any situation where this should be called - this screams an MMAP bucket to be passed down. size_t should refer to anything in memory, while off_t refers to position in a file. We have some mismatches in that. If anyone would like a nice project, cleaning up our bucket/brigade types to remove the dependency on apr_size_t/apr_off_t would be real nice before GA (create apr_bucket_size_t and apr_brigade_size_t). But, it'll be painful. That function is for backwards compat with 1.3 modules to help make it easier to port to 2.0. It does create an MMAP bucket and send it down the stack. Ryan
Re: Code questions (server/mpm_common.c)
Can this be turned into a feature macro?
Bill
Hi,
server/mpm_common.c:363
#if defined(QNX) || defined(MPE) || defined(BEOS) || defined(_OSD_POSIX) ||
defined(TPF)
|| defined(__TANDEM) || defined(OS2) ||
defined(WIN32) || defined(NETWARE)
Can I break this line into smaller chunks? If so, how?
I seem a bit rusty on how the various preprocessors handle
multiline #ifs (if they even do).
server/mpm_common.c:442
rv = apr_file_close(pod-pod_in);
if (rv != APR_SUCCESS) {
return rv;
}
return rv;
}
If we are going to waste the if, we might aswell return
APR_SUCCESS, no?
Sander
Re: Code questions (server/mpm_common.c)
Sander Striker [EMAIL PROTECTED] writes:
Hi,
server/mpm_common.c:363
#if defined(QNX) || defined(MPE) || defined(BEOS) || defined(_OSD_POSIX) ||
defined(TPF) || defined(__TANDEM) || defined(OS2) ||
defined(WIN32) || defined(NETWARE)
Can I break this line into smaller chunks? If so, how?
I seem a bit rusty on how the various preprocessors handle
multiline #ifs (if they even do).
Look in ap_config.h for the way to do it (look for SUNOS4)
Why not move this OS-specific stuff to ap_config.h and define
AP_HAVE_SUPLEMENTARY_GROUPS if we aren't on one of those platforms,
and then use AP_HAVE_SUPPLEMENTARY_GROUPS to see what to do?
(I'm assuming that the presence of setgrent() and friends isn't good
enough to make the decision.)
server/mpm_common.c:442
rv = apr_file_close(pod-pod_in);
if (rv != APR_SUCCESS) {
return rv;
}
return rv;
}
If we are going to waste the if, we might aswell return
APR_SUCCESS, no?
plenty of ways to skin a cat, all of them good
as for this code, go for the APR_SUCCESS (you're left with a trade-off
between compactness vs. using a style that can be extended if more
work is added later)
--
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
RE: Code questions (server/mpm_common.c)
From: [EMAIL PROTECTED]
[mailto:[EMAIL PROTECTED]]On Behalf Of Jeff Trawick
Sent: 11 March 2002 15:17
Sander Striker [EMAIL PROTECTED] writes:
Hi,
server/mpm_common.c:363
#if defined(QNX) || defined(MPE) || defined(BEOS) || defined(_OSD_POSIX) ||
defined(TPF) || defined(__TANDEM) || defined(OS2) ||
defined(WIN32) || defined(NETWARE)
Can I break this line into smaller chunks? If so, how?
I seem a bit rusty on how the various preprocessors handle
multiline #ifs (if they even do).
Look in ap_config.h for the way to do it (look for SUNOS4)
Why not move this OS-specific stuff to ap_config.h and define
AP_HAVE_SUPLEMENTARY_GROUPS if we aren't on one of those platforms,
and then use AP_HAVE_SUPPLEMENTARY_GROUPS to see what to do?
(I'm assuming that the presence of setgrent() and friends isn't good
enough to make the decision.)
Takers?
server/mpm_common.c:442
rv = apr_file_close(pod-pod_in);
if (rv != APR_SUCCESS) {
return rv;
}
return rv;
}
If we are going to waste the if, we might aswell return
APR_SUCCESS, no?
plenty of ways to skin a cat, all of them good
As a cat owner I sincerely how this is a figure of speech... :)
as for this code, go for the APR_SUCCESS (you're left with a trade-off
between compactness vs. using a style that can be extended if more
work is added later)
Yah. Will do. Just raising this for anyone else coding in this kind
of style.
Sander
Re: Code questions (server/mpm_common.c)
Sander Striker [EMAIL PROTECTED] writes: plenty of ways to skin a cat, all of them good As a cat owner I sincerely how this is a figure of speech... :) if only you knew our cat :) -- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: Code questions (server/mpm_common.c)
On Mon, Mar 11, 2002 at 03:33:55PM +0100, Sander Striker wrote:
...
server/mpm_common.c:442
rv = apr_file_close(pod-pod_in);
if (rv != APR_SUCCESS) {
return rv;
}
return rv;
}
If we are going to waste the if, we might aswell return
APR_SUCCESS, no?
Um. Way simpler than that:
return apr_file_close(pod-pod_in);
Forget the variable, forget the if, etc etc.
...
as for this code, go for the APR_SUCCESS (you're left with a trade-off
between compactness vs. using a style that can be extended if more
work is added later)
Um. You're talking about a single line. Go for compact for readibility.
Should the day ever arrive to extend, then it is a simple manipulation.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
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
RE: Code questions
From: Ryan Bloom [mailto:[EMAIL PROTECTED]]
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
Torching ap_document_root, WAS: RE: Code questions
From: Sander Striker [mailto:[EMAIL PROTECTED]]
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: Torching ap_document_root, WAS: RE: Code questions
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_response_code_string, WAS: RE: Code questions
From: Sander Striker [mailto:[EMAIL PROTECTED]]
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, WAS: RE: Code questions
From: Ryan Bloom [mailto:[EMAIL PROTECTED]]
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
