You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Mike Rumph <mi...@oracle.com> on 2013/12/04 20:25:32 UTC
Some redundant code and comment typos in mod_remoteip
While researching mod_remoteip to work on httpd bugs 55635 and 55637,
I noticed a few unrelated blemishes in mod_remoteip.c.
These include some redundant code and comment typos.
The attached patch against httpd trunk should address these.
Thanks,
Mike Rumph
Re: Some redundant code and comment typos in mod_remoteip
Posted by Marion & Christophe JAILLET <ch...@wanadoo.fr>.
Not correct,
this is just a french man who didn't take time to check in a
dictionary... :)
I update...
Thx
CJ
Le 13/12/2013 19:57, Mike Rumph a écrit :
> equivalant versus equivalent
> Perhaps this is a difference in British versus American spelling,
> correct?
>
> Anyway, thanks for the commits.
>
> Mike Rumph
>
> On 12/12/2013 10:12 PM, Christophe JAILLET wrote:
>> Trunk
>> =====
>> r1550650 for comments upodate
>> r1550651 for redundant check
>>
>>
>> 2.4.x
>> =====
>> r1550652 for comments upodate
>> The other one will be proposed for backport with other easy patches
>> to synch 2.4 and trunk in the coming days.
>>
>>
>> BTW, for someone who has write access to APR tree,
>> s/equivilant/equivalant/ in incluce/arch/netware/apr_private.h
>>
>> Thx,
>> CJ
>>
>>
>
>
Re: Some redundant code and comment typos in mod_remoteip
Posted by Mike Rumph <mi...@oracle.com>.
equivalant versus equivalent
Perhaps this is a difference in British versus American spelling, correct?
Anyway, thanks for the commits.
Mike Rumph
On 12/12/2013 10:12 PM, Christophe JAILLET wrote:
> Trunk
> =====
> r1550650 for comments upodate
> r1550651 for redundant check
>
>
> 2.4.x
> =====
> r1550652 for comments upodate
> The other one will be proposed for backport with other easy patches to
> synch 2.4 and trunk in the coming days.
>
>
> BTW, for someone who has write access to APR tree,
> s/equivilant/equivalant/ in incluce/arch/netware/apr_private.h
>
> Thx,
> CJ
>
>
Re: Some redundant code and comment typos in mod_remoteip
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Trunk
=====
r1550650 for comments upodate
r1550651 for redundant check
2.4.x
=====
r1550652 for comments upodate
The other one will be proposed for backport with other easy patches to
synch 2.4 and trunk in the coming days.
BTW, for someone who has write access to APR tree,
s/equivilant/equivalant/ in incluce/arch/netware/apr_private.h
Thx,
CJ
Re: Some redundant code and comment typos in mod_remoteip
Posted by Mike Rumph <mi...@oracle.com>.
Just to make things easier here are the separate patches with your ideas
included.
Thanks,
Mike Rumph
On 12/12/2013 1:37 PM, Mike Rumph wrote:
> Hello Bill,
>
> Thanks for the advice.
>
> Leaving filename as is is okay for me, I just thought I saw it split
> at other places in the code comments.
> So should I resubmit the patch or is one of the committers okay with
> picking and choosing?
> The patch overall was just some small things that I noticed and
> thought were worth mentioning.
>
> Mike Rumph
>
>
>
> On 12/12/2013 9:34 AM, William A. Rowe Jr. wrote:
>> On Wed, 04 Dec 2013 11:25:32 -0800
>> Mike Rumph <mi...@oracle.com> wrote:
>>
>>> While researching mod_remoteip to work on httpd bugs 55635 and 55637,
>>> I noticed a few unrelated blemishes in mod_remoteip.c.
>>> These include some redundant code and comment typos.
>>>
>>> The attached patch against httpd trunk should address these.
>> Some comments below, but most importantly, you might offer patches with
>> non-code, documentation fixes separately from actual code changes, for
>> the most efficient processing. Almost any committer here feels very
>> comfortable reviewing typo-correction patches, but you'll be waiting on
>> someone who feels comfortable with the specific code before your code
>> changes could be applied.
>>
>> Reordered, with feedback to specific items;
>>
>>
>> Index: modules/metadata/mod_remoteip.c
>> ===================================================================
>> --- modules/metadata/mod_remoteip.c (revision 1547874)
>> +++ modules/metadata/mod_remoteip.c (working copy)
>> @@ -37,11 +37,11 @@
>> } remoteip_proxymatch_t;
>> typedef struct {
>> - /** The header to retrieve a proxy-via ip list */
>> + /** The header to retrieve a proxy-via IP list */
>> const char *header_name;
>> /** A header to record the proxied IP's
>> * (removed as the physical connection and
>> - * from the proxy-via ip header value list)
>> + * from the proxy-via IP header value list)
>> */
>> const char *proxies_header_name;
>> /** A list of trusted proxies, ideally configured
>> @@ -53,9 +53,9 @@
>> typedef struct {
>> apr_sockaddr_t *useragent_addr;
>> char *useragent_ip;
>> - /** The list of proxy ip's ignored as remote ip's */
>> + /** The list of proxy IP's ignored as remote IP's */
>> const char *proxy_ips;
>> - /** The remaining list of untrusted proxied remote ip's */
>> + /** The remaining list of untrusted proxied remote IP's */
>> const char *proxied_remote;
>> } remoteip_req_t;
>> @@ -290,7 +290,7 @@
>> break;
>> }
>> - /* We map as IPv4 rather than IPv6 for equivilant host names
>> + /* We map as IPv4 rather than IPv6 for equivalent host names
>> * or IPV4OVERIPV6
>> */
>> rv = apr_sockaddr_info_get(&temp_sa, parse_remote,
>> @@ -309,7 +309,6 @@
>> remote = parse_remote;
>> }
>> break;
>> -
>> }
>> addrbyte = (unsigned char *) &temp_sa->sa.sin.sin_addr;
>>
>> Good changes above.
>>
>>
>>
>> @@ -198,7 +198,7 @@
>> while (!(ap_cfg_getline(lbuf, MAX_STRING_LEN, cfp))) {
>> args = lbuf;
>> while (*(arg = ap_getword_conf(cmd->temp_pool, &args)) !=
>> '\0') {
>> - if (*arg == '#' || *arg == '\0') {
>> + if (*arg == '#') {
>> break;
>> }
>> errmsg = proxies_set(cmd, cfg, arg);
>>
>> Why are you eliminating a one byte-test bypass of empty lines?
>>
>>
>> @@ -111,7 +111,7 @@
>> static int looks_like_ip(const char *ipstr)
>> {
>> if (ap_strchr_c(ipstr, ':')) {
>> - /* definitely not a hostname; assume it is intended to be an
>> IPv6 address */
>> + /* definitely not a host name; assume it is intended to be an
>> IPv6 address */ return 1;
>> }
>> Unnecessary and inaccurate doc change that most any internet project
>> would reject. hostname has a specific compsci definition. You might
>> want to add it to your conventional dictionary.
>>
>>
>> @@ -422,11 +422,11 @@
>> "which are trusted to present IP headers"),
>> AP_INIT_TAKE1("RemoteIPTrustedProxyList", proxylist_read, 0,
>> RSRC_CONF | EXEC_ON_READ,
>> - "The filename to read the list of trusted proxies, "
>> + "The file name to read the list of trusted proxies, "
>> "see the RemoteIPTrustedProxy directive"),
>> AP_INIT_TAKE1("RemoteIPInternalProxyList", proxylist_read,
>> (void*)1, RSRC_CONF | EXEC_ON_READ,
>> - "The filename to read the list of internal proxies, "
>> + "The file name to read the list of internal proxies,"
>> "see the RemoteIPTrustedProxy directive"),
>> { NULL }
>> };
>>
>> Again, a common compsci term with a well understood meaning.
>> You will find such phrases throughout the project... so your patch
>> request is essentially a request for a style change. I'm -1 on that
>> particular one.
>>
>>
>
>
>
Re: Some redundant code and comment typos in mod_remoteip
Posted by Mike Rumph <mi...@oracle.com>.
Hello Bill,
Thanks for the advice.
Leaving filename as is is okay for me, I just thought I saw it split at
other places in the code comments.
So should I resubmit the patch or is one of the committers okay with
picking and choosing?
The patch overall was just some small things that I noticed and thought
were worth mentioning.
Mike Rumph
On 12/12/2013 9:34 AM, William A. Rowe Jr. wrote:
> On Wed, 04 Dec 2013 11:25:32 -0800
> Mike Rumph <mi...@oracle.com> wrote:
>
>> While researching mod_remoteip to work on httpd bugs 55635 and 55637,
>> I noticed a few unrelated blemishes in mod_remoteip.c.
>> These include some redundant code and comment typos.
>>
>> The attached patch against httpd trunk should address these.
> Some comments below, but most importantly, you might offer patches with
> non-code, documentation fixes separately from actual code changes, for
> the most efficient processing. Almost any committer here feels very
> comfortable reviewing typo-correction patches, but you'll be waiting on
> someone who feels comfortable with the specific code before your code
> changes could be applied.
>
> Reordered, with feedback to specific items;
>
>
> Index: modules/metadata/mod_remoteip.c
> ===================================================================
> --- modules/metadata/mod_remoteip.c (revision 1547874)
> +++ modules/metadata/mod_remoteip.c (working copy)
> @@ -37,11 +37,11 @@
> } remoteip_proxymatch_t;
>
> typedef struct {
> - /** The header to retrieve a proxy-via ip list */
> + /** The header to retrieve a proxy-via IP list */
> const char *header_name;
> /** A header to record the proxied IP's
> * (removed as the physical connection and
> - * from the proxy-via ip header value list)
> + * from the proxy-via IP header value list)
> */
> const char *proxies_header_name;
> /** A list of trusted proxies, ideally configured
> @@ -53,9 +53,9 @@
> typedef struct {
> apr_sockaddr_t *useragent_addr;
> char *useragent_ip;
> - /** The list of proxy ip's ignored as remote ip's */
> + /** The list of proxy IP's ignored as remote IP's */
> const char *proxy_ips;
> - /** The remaining list of untrusted proxied remote ip's */
> + /** The remaining list of untrusted proxied remote IP's */
> const char *proxied_remote;
> } remoteip_req_t;
> @@ -290,7 +290,7 @@
> break;
> }
>
> - /* We map as IPv4 rather than IPv6 for equivilant host names
> + /* We map as IPv4 rather than IPv6 for equivalent host names
> * or IPV4OVERIPV6
> */
> rv = apr_sockaddr_info_get(&temp_sa, parse_remote,
> @@ -309,7 +309,6 @@
> remote = parse_remote;
> }
> break;
> -
> }
>
> addrbyte = (unsigned char *) &temp_sa->sa.sin.sin_addr;
>
>
> Good changes above.
>
>
>
> @@ -198,7 +198,7 @@
> while (!(ap_cfg_getline(lbuf, MAX_STRING_LEN, cfp))) {
> args = lbuf;
> while (*(arg = ap_getword_conf(cmd->temp_pool, &args)) !=
> '\0') {
> - if (*arg == '#' || *arg == '\0') {
> + if (*arg == '#') {
> break;
> }
> errmsg = proxies_set(cmd, cfg, arg);
>
> Why are you eliminating a one byte-test bypass of empty lines?
>
>
> @@ -111,7 +111,7 @@
> static int looks_like_ip(const char *ipstr)
> {
> if (ap_strchr_c(ipstr, ':')) {
> - /* definitely not a hostname; assume it is intended to be an
> IPv6 address */
> + /* definitely not a host name; assume it is intended to be an
> IPv6 address */ return 1;
> }
>
> Unnecessary and inaccurate doc change that most any internet project
> would reject. hostname has a specific compsci definition. You might
> want to add it to your conventional dictionary.
>
>
> @@ -422,11 +422,11 @@
> "which are trusted to present IP headers"),
> AP_INIT_TAKE1("RemoteIPTrustedProxyList", proxylist_read, 0,
> RSRC_CONF | EXEC_ON_READ,
> - "The filename to read the list of trusted proxies, "
> + "The file name to read the list of trusted proxies, "
> "see the RemoteIPTrustedProxy directive"),
> AP_INIT_TAKE1("RemoteIPInternalProxyList", proxylist_read,
> (void*)1, RSRC_CONF | EXEC_ON_READ,
> - "The filename to read the list of internal proxies, "
> + "The file name to read the list of internal proxies,"
> "see the RemoteIPTrustedProxy directive"),
> { NULL }
> };
>
> Again, a common compsci term with a well understood meaning.
> You will find such phrases throughout the project... so your patch
> request is essentially a request for a style change. I'm -1 on that
> particular one.
>
>
Re: Some redundant code and comment typos in mod_remoteip
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On Thu, Dec 12, 2013 at 11:34 AM, William A. Rowe Jr.
<wr...@rowe-clan.net> wrote: On Wed, 04 Dec 2013 11:25:32 -0800
Mike Rumph <mi...@oracle.com> wrote:
> > While researching mod_remoteip to work on httpd bugs 55635 and
> > I noticed a few unrelated blemishes in mod_remoteip.c.
> > These include some redundant code and comment typos.
>
> @@ -198,7 +198,7 @@
> while (!(ap_cfg_getline(lbuf, MAX_STRING_LEN, cfp))) {
> args = lbuf;
> while (*(arg = ap_getword_conf(cmd->temp_pool, &args)) !=
> '\0') {
> - if (*arg == '#' || *arg == '\0') {
> + if (*arg == '#') {
> break;
> }
> errmsg = proxies_set(cmd, cfg, arg);
>
> Why are you eliminating a one byte-test bypass of empty lines?
Never mind, I'm clear now, I see where the while condition did that
work.
Re: Some redundant code and comment typos in mod_remoteip
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On Wed, 04 Dec 2013 11:25:32 -0800
Mike Rumph <mi...@oracle.com> wrote:
> While researching mod_remoteip to work on httpd bugs 55635 and 55637,
> I noticed a few unrelated blemishes in mod_remoteip.c.
> These include some redundant code and comment typos.
>
> The attached patch against httpd trunk should address these.
Some comments below, but most importantly, you might offer patches with
non-code, documentation fixes separately from actual code changes, for
the most efficient processing. Almost any committer here feels very
comfortable reviewing typo-correction patches, but you'll be waiting on
someone who feels comfortable with the specific code before your code
changes could be applied.
Reordered, with feedback to specific items;
Index: modules/metadata/mod_remoteip.c
===================================================================
--- modules/metadata/mod_remoteip.c (revision 1547874)
+++ modules/metadata/mod_remoteip.c (working copy)
@@ -37,11 +37,11 @@
} remoteip_proxymatch_t;
typedef struct {
- /** The header to retrieve a proxy-via ip list */
+ /** The header to retrieve a proxy-via IP list */
const char *header_name;
/** A header to record the proxied IP's
* (removed as the physical connection and
- * from the proxy-via ip header value list)
+ * from the proxy-via IP header value list)
*/
const char *proxies_header_name;
/** A list of trusted proxies, ideally configured
@@ -53,9 +53,9 @@
typedef struct {
apr_sockaddr_t *useragent_addr;
char *useragent_ip;
- /** The list of proxy ip's ignored as remote ip's */
+ /** The list of proxy IP's ignored as remote IP's */
const char *proxy_ips;
- /** The remaining list of untrusted proxied remote ip's */
+ /** The remaining list of untrusted proxied remote IP's */
const char *proxied_remote;
} remoteip_req_t;
@@ -290,7 +290,7 @@
break;
}
- /* We map as IPv4 rather than IPv6 for equivilant host names
+ /* We map as IPv4 rather than IPv6 for equivalent host names
* or IPV4OVERIPV6
*/
rv = apr_sockaddr_info_get(&temp_sa, parse_remote,
@@ -309,7 +309,6 @@
remote = parse_remote;
}
break;
-
}
addrbyte = (unsigned char *) &temp_sa->sa.sin.sin_addr;
Good changes above.
@@ -198,7 +198,7 @@
while (!(ap_cfg_getline(lbuf, MAX_STRING_LEN, cfp))) {
args = lbuf;
while (*(arg = ap_getword_conf(cmd->temp_pool, &args)) !=
'\0') {
- if (*arg == '#' || *arg == '\0') {
+ if (*arg == '#') {
break;
}
errmsg = proxies_set(cmd, cfg, arg);
Why are you eliminating a one byte-test bypass of empty lines?
@@ -111,7 +111,7 @@
static int looks_like_ip(const char *ipstr)
{
if (ap_strchr_c(ipstr, ':')) {
- /* definitely not a hostname; assume it is intended to be an
IPv6 address */
+ /* definitely not a host name; assume it is intended to be an
IPv6 address */ return 1;
}
Unnecessary and inaccurate doc change that most any internet project
would reject. hostname has a specific compsci definition. You might
want to add it to your conventional dictionary.
@@ -422,11 +422,11 @@
"which are trusted to present IP headers"),
AP_INIT_TAKE1("RemoteIPTrustedProxyList", proxylist_read, 0,
RSRC_CONF | EXEC_ON_READ,
- "The filename to read the list of trusted proxies, "
+ "The file name to read the list of trusted proxies, "
"see the RemoteIPTrustedProxy directive"),
AP_INIT_TAKE1("RemoteIPInternalProxyList", proxylist_read,
(void*)1, RSRC_CONF | EXEC_ON_READ,
- "The filename to read the list of internal proxies, "
+ "The file name to read the list of internal proxies,"
"see the RemoteIPTrustedProxy directive"),
{ NULL }
};
Again, a common compsci term with a well understood meaning.
You will find such phrases throughout the project... so your patch
request is essentially a request for a style change. I'm -1 on that
particular one.
Re: [PATCH]Some redundant code and comment typos in mod_remoteip
Posted by Mike Rumph <mi...@oracle.com>.
I forgot to add a [PATCH] tag on the front of the subject.
The changes here are minor, but they do make the code a little cleaner.
On 12/4/2013 11:25 AM, Mike Rumph wrote:
> While researching mod_remoteip to work on httpd bugs 55635 and 55637,
> I noticed a few unrelated blemishes in mod_remoteip.c.
> These include some redundant code and comment typos.
>
> The attached patch against httpd trunk should address these.
>
> Thanks,
>
> Mike Rumph