Change the ap_cfg_getline() and ap_cfg_getc() to return an error code.

Also:
- Make ap_cfg_getline() return APR_ENOSPC if a config line is too long.
- Add ap_pcfg_strerror() function to convert ap_cfg_getline's return value
  into a nice message.
- Adjust definition of ap_configfile_t accordingly.

Not bumping MMN because it has already been bumped today.


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1086756 13f79535-47bb-0310-9956-ffa450edef68

docs/manual/developer/new_api_2_4.xml

@@ -101,6 +101,8 @@
       <li>New API to retain data across module unload/load</li>
       <li>New check_config hook</li>
       <li>New ap_process_fnmatch_configs() to process wildcards</li>
+      <li>Change ap_configfile_t, ap_cfg_getline(), ap_cfg_getc() to return error
+          code, new ap_pcfg_strerror().</li>
     </ul>
   </section>
 

include/ap_mmn.h

@@ -311,6 +311,8 @@
                            connectionPoolTTL (connection_pool_ttl, int->apr_interval_t)
  * 20110329.0 (2.3.12-dev) Change single-bit signed fields to unsigned in
  *                         proxy and cache interfaces.
+ *                         Change ap_configfile_t/ap_cfg_getline()/
+ *                         ap_cfg_getc() API, add ap_pcfg_strerror()
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */

include/http_config.h

@@ -257,13 +257,18 @@ struct command_struct {
 /** Common structure for reading of config files / passwd files etc. */
 typedef struct ap_configfile_t ap_configfile_t;
 struct ap_configfile_t {
-    int (*getch) (void *param);	    /**< a getc()-like function */
-    void *(*getstr) (void *buf, size_t bufsiz, void *param);
-				    /**< a fgets()-like function */
-    int (*close) (void *param);	    /**< a close handler function */
-    void *param;                    /**< the argument passed to getch/getstr/close */
-    const char *name;               /**< the filename / description */
-    unsigned line_number;           /**< current line number, starting at 1 */
+    /**< an apr_file_getc()-like function */
+    apr_status_t (*getch) (char *ch, void *param);
+    /**< an apr_file_gets()-like function */
+    apr_status_t (*getstr) (void *buf, size_t bufsiz, void *param);
+    /**< a close handler function */
+    apr_status_t (*close) (void *param);
+    /**< the argument passed to getch/getstr/close */
+    void *param;
+    /**< the filename / description */
+    const char *name;
+    /**< current line number, starting at 1 */
+    unsigned line_number;
 };
 
 /**
@@ -765,25 +770,26 @@ AP_DECLARE(apr_status_t) ap_pcfg_openfile(ap_configfile_t **ret_cfg,
 AP_DECLARE(ap_configfile_t *) ap_pcfg_open_custom(apr_pool_t *p, 
     const char *descr,
     void *param,
-    int(*getc_func)(void*),
-    void *(*gets_func) (void *buf, size_t bufsiz, void *param),
-    int(*close_func)(void *param));
+    apr_status_t (*getc_func) (char *ch, void *param),
+    apr_status_t (*gets_func) (void *buf, size_t bufsiz, void *param),
+    apr_status_t (*close_func) (void *param));
 
 /**
- * Read one line from open ap_configfile_t, strip LF, increase line number
+ * Read one line from open ap_configfile_t, strip leading and trailing
+ * whitespace, increase line number
  * @param buf place to store the line read
  * @param bufsize size of the buffer
  * @param cfp File to read from
- * @return 1 on success, 0 on failure
+ * @return error status, APR_ENOSPC if bufsize is too small for the line
  */
-AP_DECLARE(int) ap_cfg_getline(char *buf, size_t bufsize, ap_configfile_t *cfp);
+AP_DECLARE(apr_status_t) ap_cfg_getline(char *buf, size_t bufsize, ap_configfile_t *cfp);
 
 /**
  * Read one char from open configfile_t, increase line number upon LF 
  * @param cfp The file to read from
  * @return the character read
  */
-AP_DECLARE(int) ap_cfg_getc(ap_configfile_t *cfp);
+AP_DECLARE(apr_status_t) ap_cfg_getc(char *ch, ap_configfile_t *cfp);
 
 /**
  * Detach from open ap_configfile_t, calling the close handler
@@ -792,6 +798,16 @@ AP_DECLARE(int) ap_cfg_getc(ap_configfile_t *cfp);
  */
 AP_DECLARE(int) ap_cfg_closefile(ap_configfile_t *cfp);
 
+/**
+ * Convert a return value from ap_cfg_getline or ap_cfg_getc to a user friendly
+ * string.
+ * @param p The pool to allocate the string from
+ * @param cfp The config file
+ * @return The error string, NULL if rc == APR_SUCCESS
+ */
+AP_DECLARE(const char *) ap_pcfg_strerror(apr_pool_t *p, ap_configfile_t *cfp,
+                                          apr_status_t rc);
+
 /**
  * Read all data between the current &lt;foo&gt; and the matching &lt;/foo&gt;.  All
  * of this data is forgotten immediately.  

modules/lua/mod_lua.c

@@ -317,8 +317,8 @@ static apr_size_t config_getstr(ap_configfile_t *cfg, char *buf,
     apr_size_t i = 0;
 
     if (cfg->getstr) {
-        const char *res = (cfg->getstr) (buf, bufsiz, cfg->param);
-        if (res) {
+        apr_status_t rc = (cfg->getstr) (buf, bufsiz, cfg->param);
+        if (rc == APR_SUCCESS) {
             i = strlen(buf);
             if (i && buf[i - 1] == '\n')
                 ++cfg->line_number;
@@ -330,8 +330,9 @@ static apr_size_t config_getstr(ap_configfile_t *cfg, char *buf,
     }
     else {
         while (i < bufsiz) {
-            int ch = (cfg->getch) (cfg->param);
-            if (ch == EOF)
+            char ch;
+            apr_status_t rc = (cfg->getch) (&ch, cfg->param);
+            if (rc != APR_SUCCESS)
                 break;
             buf[i++] = ch;
             if (ch == '\n') {

server/config.c

@@ -1169,6 +1169,7 @@ AP_DECLARE(const char *) ap_build_cont_config(apr_pool_t *p,
     char *bracket;
     const char *retval;
     ap_directive_t *sub_tree = NULL;
+    apr_status_t rc;
 
     /* Since this function can be called recursively, allocate
      * the temporary 8k string buffer from the temp_pool rather
@@ -1177,7 +1178,8 @@ AP_DECLARE(const char *) ap_build_cont_config(apr_pool_t *p,
     l = apr_palloc(temp_pool, MAX_STRING_LEN);
 
     bracket = apr_pstrcat(temp_pool, orig_directive + 1, ">", NULL);
-    while (!(ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))) {
+    while ((rc = ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))
+           == APR_SUCCESS) {
         if (!memcmp(l, "</", 2)
             && (strcasecmp(l + 2, bracket) == 0)
             && (*curr_parent == NULL)) {
@@ -1196,6 +1198,8 @@ AP_DECLARE(const char *) ap_build_cont_config(apr_pool_t *p,
             sub_tree = *current;
         }
     }
+    if (rc != APR_EOF && rc != APR_SUCCESS)
+        return ap_pcfg_strerror(temp_pool, parms->config_file, rc);
 
     *current = sub_tree;
     return NULL;
@@ -1290,6 +1294,7 @@ AP_DECLARE(const char *) ap_build_config(cmd_parms *parms,
     char *l = apr_palloc (temp_pool, MAX_STRING_LEN);
     const char *errmsg;
     ap_directive_t **last_ptr = NULL;
+    apr_status_t rc;
 
     if (current != NULL) {
         /* If we have to traverse the whole tree again for every included
@@ -1313,7 +1318,8 @@ AP_DECLARE(const char *) ap_build_config(cmd_parms *parms,
         }
     }
 
-    while (!(ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))) {
+    while ((rc = ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))
+           == APR_SUCCESS) {
         errmsg = ap_build_config_sub(p, temp_pool, l, parms,
                                      &current, &curr_parent, conftree);
         if (errmsg != NULL)
@@ -1327,6 +1333,8 @@ AP_DECLARE(const char *) ap_build_config(cmd_parms *parms,
             *conftree = current;
         }
     }
+    if (rc != APR_EOF && rc != APR_SUCCESS)
+        return ap_pcfg_strerror(temp_pool, parms->config_file, rc);
 
     if (curr_parent != NULL) {
         errmsg = "";
@@ -1499,8 +1507,10 @@ AP_DECLARE(const char *) ap_soak_end_container(cmd_parms *cmd, char *directive)
     char l[MAX_STRING_LEN];
     const char *args;
     char *cmd_name;
+    apr_status_t rc;
 
-    while(!(ap_cfg_getline(l, MAX_STRING_LEN, cmd->config_file))) {
+    while((rc = ap_cfg_getline(l, MAX_STRING_LEN, cmd->config_file))
+          == APR_SUCCESS) {
 #if RESOLVE_ENV_PER_TOKEN
         args = l;
 #else
@@ -1533,6 +1543,8 @@ AP_DECLARE(const char *) ap_soak_end_container(cmd_parms *cmd, char *directive)
             }
         }
     }
+    if (rc != APR_EOF && rc != APR_SUCCESS)
+        return ap_pcfg_strerror(cmd->temp_pool, cmd->config_file, rc);
 
     return apr_pstrcat(cmd->pool, "Expected </",
                        directive + 1, "> before end of configuration",
@@ -1592,31 +1604,31 @@ typedef struct {
 
 
 /* arr_elts_getstr() returns the next line from the string array. */
-static void *arr_elts_getstr(void *buf, size_t bufsiz, void *param)
+static apr_status_t arr_elts_getstr(void *buf, size_t bufsiz, void *param)
 {
     arr_elts_param_t *arr_param = (arr_elts_param_t *)param;
+    char *elt;
 
     /* End of array reached? */
     if (++arr_param->curr_idx > arr_param->array->nelts)
-        return NULL;
+        return APR_EOF;
 
     /* return the line */
-    apr_cpystrn(buf,
-                ((char **)arr_param->array->elts)[arr_param->curr_idx - 1],
-                bufsiz);
-
-    return buf;
+    elt = ((char **)arr_param->array->elts)[arr_param->curr_idx - 1];
+    if (apr_cpystrn(buf, elt, bufsiz) - (char *)buf >= bufsiz - 1)
+        return APR_ENOSPC;
+    return APR_SUCCESS;
 }
 
 
 /* arr_elts_close(): dummy close routine (makes sure no more lines can be read) */
-static int arr_elts_close(void *param)
+static apr_status_t arr_elts_close(void *param)
 {
     arr_elts_param_t *arr_param = (arr_elts_param_t *)param;
 
     arr_param->curr_idx = arr_param->array->nelts;
 
-    return 0;
+    return APR_SUCCESS;
 }
 
 static const char *process_command_config(server_rec *s,
@@ -1700,9 +1712,12 @@ AP_DECLARE(const char *) ap_process_resource_config(server_rec *s,
     ap_cfg_closefile(cfp);
 
     if (error) {
+        if (parms.err_directive)
             return apr_psprintf(p, "Syntax error on line %d of %s: %s",
                                 parms.err_directive->line_num,
                                 parms.err_directive->filename, error);
+        else
+            return error;
     }
 
     return NULL;
@@ -1956,6 +1971,7 @@ AP_DECLARE(int) ap_process_config_tree(server_rec *s,
 
     errmsg = ap_walk_config(conftree, &parms, s->lookup_defaults);
     if (errmsg) {
+        if (parms.err_directive)
             ap_log_perror(APLOG_MARK, APLOG_STARTUP, 0, p,
                           "Syntax error on line %d of %s:",
                           parms.err_directive->line_num,

server/util.c

@@ -766,30 +766,20 @@ AP_DECLARE(int) ap_cfg_closefile(ap_configfile_t *cfp)
     return (cfp->close == NULL) ? 0 : cfp->close(cfp->param);
 }
 
+/* we can't use apr_file_* directly because of linking issues on Windows */
 static apr_status_t cfg_close(void *param)
 {
-    apr_file_t *cfp = (apr_file_t *) param;
-    return (apr_file_close(cfp));
+    return apr_file_close(param);
 }
 
-static int cfg_getch(void *param)
+static apr_status_t cfg_getch(char *ch, void *param)
 {
-    char ch;
-    apr_file_t *cfp = (apr_file_t *) param;
-    if (apr_file_getc(&ch, cfp) == APR_SUCCESS)
-        return ch;
-    return (int)EOF;
+    return apr_file_getc(ch, param);
 }
 
-static void *cfg_getstr(void *buf, size_t bufsiz, void *param)
+static apr_status_t cfg_getstr(void *buf, size_t bufsiz, void *param)
 {
-    apr_file_t *cfp = (apr_file_t *) param;
-    apr_status_t rv;
-    rv = apr_file_gets(buf, bufsiz, cfp);
-    if (rv == APR_SUCCESS) {
-        return buf;
-    }
-    return NULL;
+    return apr_file_gets(buf, bufsiz, param);
 }
 
 /* Open a ap_configfile_t as FILE, return open ap_configfile_t struct pointer */
@@ -864,9 +854,9 @@ AP_DECLARE(apr_status_t) ap_pcfg_openfile(ap_configfile_t **ret_cfg,
     new_cfg = apr_palloc(p, sizeof(*new_cfg));
     new_cfg->param = file;
     new_cfg->name = apr_pstrdup(p, name);
-    new_cfg->getch = (int (*)(void *)) cfg_getch;
-    new_cfg->getstr = (void *(*)(void *, size_t, void *)) cfg_getstr;
-    new_cfg->close = (int (*)(void *)) cfg_close;
+    new_cfg->getch = cfg_getch;
+    new_cfg->getstr = cfg_getstr;
+    new_cfg->close = cfg_close;
     new_cfg->line_number = 0;
     *ret_cfg = new_cfg;
     return APR_SUCCESS;
@@ -874,66 +864,83 @@ AP_DECLARE(apr_status_t) ap_pcfg_openfile(ap_configfile_t **ret_cfg,
 
 
 /* Allocate a ap_configfile_t handle with user defined functions and params */
-AP_DECLARE(ap_configfile_t *) ap_pcfg_open_custom(apr_pool_t *p,
-                       const char *descr,
-                       void *param,
-                       int(*getch)(void *param),
-                       void *(*getstr) (void *buf, size_t bufsiz, void *param),
-                       int(*close_func)(void *param))
+AP_DECLARE(ap_configfile_t *) ap_pcfg_open_custom(
+            apr_pool_t *p, const char *descr, void *param,
+            apr_status_t (*getc_func) (char *ch, void *param),
+            apr_status_t (*gets_func) (void *buf, size_t bufsize, void *param),
+            apr_status_t (*close_func) (void *param))
 {
     ap_configfile_t *new_cfg = apr_palloc(p, sizeof(*new_cfg));
-#ifdef DEBUG
-    ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
-                 "Opening config handler %s", descr);
-#endif
     new_cfg->param = param;
     new_cfg->name = descr;
-    new_cfg->getch = getch;
-    new_cfg->getstr = getstr;
+    new_cfg->getch = getc_func;
+    new_cfg->getstr = gets_func;
     new_cfg->close = close_func;
     new_cfg->line_number = 0;
     return new_cfg;
 }
 
 /* Read one character from a configfile_t */
-AP_DECLARE(int) ap_cfg_getc(ap_configfile_t *cfp)
+AP_DECLARE(apr_status_t) ap_cfg_getc(char *ch, ap_configfile_t *cfp)
 {
-    register int ch = cfp->getch(cfp->param);
-    if (ch == LF)
+    apr_status_t rc = cfp->getch(ch, cfp->param);
+    if (rc == APR_SUCCESS && *ch == LF)
         ++cfp->line_number;
-    return ch;
+    return rc;
+}
+
+AP_DECLARE(const char *) ap_pcfg_strerror(apr_pool_t *p, ap_configfile_t *cfp,
+                                          apr_status_t rc)
+{
+    char buf[MAX_STRING_LEN];
+    if (rc == APR_SUCCESS)
+        return NULL;
+    return apr_psprintf(p, "Error reading %s at line %d: %s",
+                        cfp->name, cfp->line_number,
+                        rc == APR_ENOSPC ? "Line too long"
+                                         : apr_strerror(rc, buf, sizeof(buf)));
 }
 
 /* Read one line from open ap_configfile_t, strip LF, increase line number */
 /* If custom handler does not define a getstr() function, read char by char */
-AP_DECLARE(int) ap_cfg_getline(char *buf, size_t bufsize, ap_configfile_t *cfp)
+AP_DECLARE(apr_status_t) ap_cfg_getline(char *buf, size_t bufsize, ap_configfile_t *cfp)
 {
+    apr_status_t rc;
+    char *src, *dst;
     /* If a "get string" function is defined, use it */
     if (cfp->getstr != NULL) {
-        char *src, *dst;
         char *cp;
         char *cbuf = buf;
         size_t cbufsize = bufsize;
 
         while (1) {
             ++cfp->line_number;
-            if (cfp->getstr(cbuf, cbufsize, cfp->param) == NULL)
-                return 1;
+            rc = cfp->getstr(cbuf, cbufsize, cfp->param);
+            if (rc == APR_EOF) {
+                if (cbuf != buf) {
+		    *cbuf = '\0';
+                    break;
+		}
+                else {
+                    return APR_EOF;
+		}
+            }
+            if (rc != APR_SUCCESS) {
+                return rc;
+            }
 
             /*
              *  check for line continuation,
              *  i.e. match [^\\]\\[\r]\n only
              */
             cp = cbuf;
-            while (cp < cbuf+cbufsize && *cp != '\0')
-                cp++;
+            cp += strlen(cp);
             if (cp > cbuf && cp[-1] == LF) {
                 cp--;
                 if (cp > cbuf && cp[-1] == CR)
                     cp--;
                 if (cp > cbuf && cp[-1] == '\\') {
                     cp--;
-                    if (!(cp > cbuf && cp[-1] == '\\')) {
                     /*
                      * line continuation requested -
                      * then remove backslash and continue
@@ -942,18 +949,52 @@ AP_DECLARE(int) ap_cfg_getline(char *buf, size_t bufsize, ap_configfile_t *cfp)
                     cbuf = cp;
                     continue;
                 }
-                    else {
-                        /*
-                         * no real continuation because escaped -
-                         * then just remove escape character
-                         */
-                        for ( ; cp < cbuf+cbufsize && *cp != '\0'; cp++)
-                            cp[0] = cp[1];
-                    }
             }
+            else if (cp - buf >= bufsize - 1) {
+                return APR_ENOSPC;
             }
             break;
         }
+    } else {
+        /* No "get string" function defined; read character by character */
+        size_t i = 0;
+
+        if (bufsize < 2) {
+            /* too small, assume caller is crazy */
+            return APR_EINVAL;
+        }
+        buf[0] = '\0';
+
+        while (1) {
+            char c;
+            rc = cfp->getch(&c, cfp->param);
+            if (rc == APR_EOF) {
+                if (i > 0)
+                    break;
+                else
+                    return APR_EOF;
+            }
+            if (rc != APR_SUCCESS)
+                return rc;
+            if (c == LF) {
+                ++cfp->line_number;
+                /* check for line continuation */
+                if (i > 0 && buf[i-1] == '\\') {
+                    i--;
+                    continue;
+                }
+                else {
+                    break;
+                }
+            }
+            else if (i >= bufsize - 2) {
+                return APR_ENOSPC;
+            }
+            buf[i] = c;
+            ++i;
+        }
+	buf[i] = '\0';
+    }
 
     /*
      * Leading and trailing white space is eliminated completely
@@ -967,77 +1008,12 @@ AP_DECLARE(int) ap_cfg_getline(char *buf, size_t bufsize, ap_configfile_t *cfp)
         *dst = '\0';
     /* Zap leading whitespace by shifting */
     if (src != buf)
-            for (dst = buf; (*dst++ = *src++) != '\0'; )
-                ;
+        memmove(buf, src, dst - src + 2);
 
 #ifdef DEBUG_CFG_LINES
-        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL, "Read config: %s", buf);
+    ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, NULL, "Read config: '%s'", buf);
 #endif
-        return 0;
-    } else {
-        /* No "get string" function defined; read character by character */
-        register int c;
-        register size_t i = 0;
-
-        buf[0] = '\0';
-        /* skip leading whitespace */
-        do {
-            c = cfp->getch(cfp->param);
-        } while (c == '\t' || c == ' ');
-
-        if (c == EOF)
-            return 1;
-
-        if(bufsize < 2) {
-            /* too small, assume caller is crazy */
-            return 1;
-        }
-
-        while (1) {
-            if ((c == '\t') || (c == ' ')) {
-                buf[i++] = ' ';
-                while ((c == '\t') || (c == ' '))
-                    c = cfp->getch(cfp->param);
-            }
-            if (c == CR) {
-                /* silently ignore CR (_assume_ that a LF follows) */
-                c = cfp->getch(cfp->param);
-            }
-            if (c == LF) {
-                /* increase line number and return on LF */
-                ++cfp->line_number;
-            }
-            if (c == EOF || c == 0x4 || c == LF || i >= (bufsize - 2)) {
-                /*
-                 *  check for line continuation
-                 */
-                if (i > 0 && buf[i-1] == '\\') {
-                    i--;
-                    if (!(i > 0 && buf[i-1] == '\\')) {
-                        /* line is continued */
-                        c = cfp->getch(cfp->param);
-                        continue;
-                    }
-                    /* else nothing needs be done because
-                     * then the backslash is escaped and
-                     * we just strip to a single one
-                     */
-                }
-                /* blast trailing whitespace */
-                while (i > 0 && apr_isspace(buf[i - 1]))
-                    --i;
-                buf[i] = '\0';
-#ifdef DEBUG_CFG_LINES
-                ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
-                             "Read config: %s", buf);
-#endif
-                return 0;
-            }
-            buf[i] = c;
-            ++i;
-            c = cfp->getch(cfp->param);
-        }
-    }
+    return APR_SUCCESS;
 }
 
 /* Size an HTTP header field list item, as separated by a comma.