Re: Some redundant code and comment typos in mod_remoteip

[Mike Rumph]

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 <mike.ru...@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.