httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <trawi...@bellsouth.net>
Subject Re: [PATCH] improving line number reporting for config file syntax errors
Date Mon, 29 May 2000 22:14:55 GMT
> From: rbb@covalent.net
> Mailing-List: contact new-httpd-help@apache.org; run by ezmlm
> Precedence: bulk
> X-No-Archive: yes
> Reply-To: new-httpd@apache.org
> list-help: <mailto:new-httpd-help@apache.org>
> list-unsubscribe: <mailto:new-httpd-unsubscribe@apache.org>
> list-post: <mailto:new-httpd@apache.org>
> X-Authentication-Warning: koj.rkbloom.net: rbb owned process doing -bs
> Date: Mon, 29 May 2000 14:37:01 -0700 (PDT)
> X-Sender: rbb@koj.rkbloom.net
> Content-Type: TEXT/PLAIN; charset=US-ASCII
> X-Spam-Rating: locus.apache.org 1.6.2 0/1000/N
> X-UIDL: 5b9e19ce9ba2e3b3a5225bee159ca153
> 
> 
> Here is a real quick update to my last patch for this problem that solves
> syntax errors in the global context.  The patch I originally posted had
> only been tested with errors inside a context and that error had to be an
> unrecognized command.  This one has caught and correctly reported the line
> number for:
> 
> PidFile 1 2 3 4 5 6 7
> 
> And
> 
> <Directory Foo>
> silly
> </Directory>
> 

Yep, your patch passes my testcases.  See my much larger!! patch below
which also passes the testcases.  (I won't bother posting them since
both patches work fine with the testcases.)

> I like this patch because it is relatively clean with regard to how much
> it changes things.  The basic idea to this patch, is that we pass around
> the configfile_t variable that we use when reporting the error.  At the
> time that we actually receive an error while reading the config file, we
> just replace the line number in the configfile_t with the line number that
> caused the error.
> 
> Ryan

The basic idea of my patch is that we pass back a ptr to the
ap_directive_t where we noticed the problem.  I don't know what we'd
ever need besides file+line number, but it is convenient as-is and may
be useful in the future.

I like your patch for the brevity, but I get bad vibes from the way
the info is passed back.  Right now, I get bad vibes from my patch
because of the amount of changes required (albeit simple changes) but
I like the way the information is passed back.

Note: My patch does not pass the address of the bad ap_directive_t to
the caller of ap_build_cont_config().  I don't think such detail is
reported from there anyway. 

Note also that the externalized routine ap_limit_section() has an
additional parameter.  Comments state that it is externalized for
mod_perl.

Have fun,

Jeff

Index: src/include/http_config.h
===================================================================
RCS file: /cvs/apache/apache-2.0/src/include/http_config.h,v
retrieving revision 1.26
diff -u -r1.26 http_config.h
--- http_config.h	2000/05/28 03:52:36	1.26
+++ http_config.h	2000/05/29 22:08:08
@@ -93,7 +93,8 @@
     TAKE3,			/* three arguments only */
     TAKE23,			/* two or three arguments */
     TAKE123,			/* one, two or three arguments */
-    TAKE13			/* one or three arguments */
+    TAKE13,			/* one or three arguments */
+    RAW_ARGS_D                  /* like RAW_ARGS, but with ap_directive_t */
 };
 
 typedef struct command_struct {
@@ -349,6 +350,7 @@
 					 ap_pool_t *temp_pool,
 					 ap_directive_t **conftree);
 API_EXPORT(const char *) ap_walk_config(ap_directive_t *conftree,
+                                        ap_directive_t **bad_directive,
 					cmd_parms *parms, void *config);
 
 /* ap_check_cmd_context() definitions: */
Index: src/include/http_core.h
===================================================================
RCS file: /cvs/apache/apache-2.0/src/include/http_core.h,v
retrieving revision 1.11
diff -u -r1.11 http_core.h
--- http_core.h	2000/05/27 22:40:20	1.11
+++ http_core.h	2000/05/29 22:08:09
@@ -301,7 +301,8 @@
 CORE_EXPORT(void) ap_add_per_dir_conf (server_rec *s, void *dir_config);
 CORE_EXPORT(void) ap_add_per_url_conf (server_rec *s, void *url_config);
 CORE_EXPORT(void) ap_add_file_conf(core_dir_config *conf, void *url_config);
-CORE_EXPORT_NONSTD(const char *) ap_limit_section (cmd_parms *cmd, void *dummy, const char
*arg);
+CORE_EXPORT_NONSTD(const char *) ap_limit_section (cmd_parms *cmd, void *dummy, const char
*arg,
+                                                   ap_directive_t **bad_directive);
 
 #endif
 
Index: src/main/http_config.c
===================================================================
RCS file: /cvs/apache/apache-2.0/src/main/http_config.c,v
retrieving revision 1.54
diff -u -r1.54 http_config.c
--- http_config.c	2000/05/28 03:52:41	1.54
+++ http_config.c	2000/05/29 22:08:12
@@ -631,7 +631,8 @@
  */
 
 static const char *invoke_cmd(const command_rec *cmd, cmd_parms *parms,
-			    void *mconfig, const char *args)
+                              void *mconfig, const char *args, 
+                              ap_directive_t **bad_directive)
 {
     char *w, *w2, *w3;
     const char *errmsg;
@@ -650,6 +651,13 @@
 	return ((const char *(*)(cmd_parms *, void *, const char *))
 		(cmd->func)) (parms, mconfig, args);
 
+    case RAW_ARGS_D:
+#ifdef RESOLVE_ENV_PER_TOKEN
+	args = ap_resolve_env(parms->pool,args);
+#endif
+	return ((const char *(*)(cmd_parms *, void *, const char *, ap_directive_t **))
+		(cmd->func)) (parms, mconfig, args, bad_directive);
+
     case NO_ARGS:
 	if (*args != 0)
 	    return ap_pstrcat(parms->pool, cmd->name, " takes no arguments",
@@ -841,13 +849,15 @@
 }
 
 static const char *execute_now(char *cmd_line, const char *args, cmd_parms *parms, 
-                         ap_pool_t *p, ap_pool_t *ptemp,
-                         ap_directive_t **sub_tree, ap_directive_t *parent);
+                               ap_pool_t *p, ap_pool_t *ptemp,
+                               ap_directive_t **sub_tree, ap_directive_t *parent,
+                               ap_directive_t **bad_directive);
 
 static const char * ap_build_config_sub(ap_pool_t *p, ap_pool_t *temp_pool,
 					const char *l, cmd_parms *parms,
 					ap_directive_t **current,
-					ap_directive_t **curr_parent)
+					ap_directive_t **curr_parent,
+                                        ap_directive_t **bad_directive)
 {
     const char *args;
     char *cmd_name;
@@ -877,7 +887,7 @@
             ap_directive_t *sub_tree = NULL;
 
             retval = execute_now(cmd_name, args, parms, p, temp_pool, 
-                                 &sub_tree, *curr_parent);
+                                 &sub_tree, *curr_parent, bad_directive);
             if (*current) {
                 (*current)->next = sub_tree;
             }
@@ -944,15 +954,16 @@
 }
 
 const char * ap_build_cont_config(ap_pool_t *p, ap_pool_t *temp_pool,
-					cmd_parms *parms,
-					ap_directive_t **current,
-					ap_directive_t **curr_parent,
-                                        char *orig_directive)
+                                  cmd_parms *parms,
+                                  ap_directive_t **current,
+                                  ap_directive_t **curr_parent,
+                                  char *orig_directive)
 {
     char l[MAX_STRING_LEN];
     char *bracket;
     const char *retval;
     ap_directive_t *conftree = NULL;
+    ap_directive_t *bad_directive = NULL;
 
     bracket = ap_pstrcat(p, orig_directive + 1, ">", NULL);
     while(!(ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))) {
@@ -961,7 +972,7 @@
             break;
         } 
         retval = ap_build_config_sub(p, temp_pool, l, parms, current, 
-                                     curr_parent);
+                                     curr_parent, &bad_directive);
         if (retval != NULL)
             return retval;
         if (conftree == NULL && curr_parent != NULL) { 
@@ -976,7 +987,8 @@
 }
 
 static const char *ap_walk_config_sub(const ap_directive_t *current,
-				      cmd_parms *parms, void *config)
+				      cmd_parms *parms, void *config,
+                                      ap_directive_t **bad_directive)
 {
     module *mod = top_module;
 
@@ -994,8 +1006,8 @@
 	    void *mconfig = ap_set_config_vectors(parms,config, mod);
 	    const char *retval;
 
-	    retval = invoke_cmd(cmd, parms, mconfig, current->args);
-	    if (retval == NULL || strcmp(retval, DECLINE_CMD) != 0)
+	    retval = invoke_cmd(cmd, parms, mconfig, current->args, bad_directive);
+            if (retval == NULL || strcmp(retval, DECLINE_CMD) != 0)
 		return retval;
 
 	    mod = mod->next;	/* Next time around, skip this one */
@@ -1005,11 +1017,13 @@
 }
 
 API_EXPORT(const char *) ap_walk_config(ap_directive_t *current,
+                                        ap_directive_t **bad_directive,
 					cmd_parms *parms, void *config)
 {
     void *oldconfig = parms->context;
 
     parms->context = config;
+    *bad_directive = NULL;
 
     /* scan through all directives, executing each one */
     for (; current != NULL; current = current->next) {
@@ -1018,8 +1032,11 @@
 	parms->directive = current;
 
         /* actually parse the command and execute the correct function */
-        errmsg = ap_walk_config_sub(current, parms, config);
+        errmsg = ap_walk_config_sub(current, parms, config, bad_directive);
 	if (errmsg != NULL) {
+            if (*bad_directive == NULL) {
+                *bad_directive = current;
+            }
 	    /* restore the context (just in case) */
 	    parms->context = oldconfig;
 	    return errmsg;
@@ -1037,6 +1054,7 @@
 {
     ap_directive_t *current = NULL;
     ap_directive_t *curr_parent = NULL;
+    ap_directive_t *bad_directive = NULL;
     char l[MAX_STRING_LEN];
     const char *errmsg;
 
@@ -1045,7 +1063,7 @@
     while (!(ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))) {
 
 	errmsg = ap_build_config_sub(p, temp_pool, l, parms,
-				     &current, &curr_parent);
+				     &current, &curr_parent, &bad_directive);
 	if (errmsg != NULL)
 	    return errmsg;
 
@@ -1171,8 +1189,9 @@
 }
 
 static const char *execute_now(char *cmd_line, const char *args, cmd_parms *parms, 
-                         ap_pool_t *p, ap_pool_t *ptemp, 
-                         ap_directive_t **sub_tree, ap_directive_t *parent)
+                               ap_pool_t *p, ap_pool_t *ptemp, 
+                               ap_directive_t **sub_tree, ap_directive_t *parent,
+                               ap_directive_t **bad_directive)
 {
     module *mod = top_module;
     const command_rec *cmd;
@@ -1185,7 +1204,7 @@
                           NULL);
     }
     else {
-        return invoke_cmd(cmd, parms, sub_tree, args);
+        return invoke_cmd(cmd, parms, sub_tree, args, bad_directive);
     }
 }
 
@@ -1235,7 +1254,7 @@
     const char *errmsg;
     cmd_parms parms;
     arr_elts_param_t arr_parms;
-    ap_directive_t *conftree;
+    ap_directive_t *conftree, *bad_directive;
 
     arr_parms.curr_idx = 0;
     arr_parms.array = arr;
@@ -1252,7 +1271,8 @@
 
     errmsg = ap_build_config(&parms, p, ptemp, &conftree);
     if (errmsg == NULL)
-	errmsg = ap_walk_config(conftree, &parms, s->lookup_defaults);
+	errmsg = ap_walk_config(conftree, &bad_directive, &parms, 
+                                s->lookup_defaults);
     if (errmsg) {
         ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL,
                      "Syntax error in -C/-c directive:\n%s", errmsg);
@@ -1266,7 +1286,7 @@
 {
     cmd_parms parms;
     ap_finfo_t finfo;
-    ap_directive_t *conftree;
+    ap_directive_t *conftree, *bad_directive;
     const char *errmsg;
     configfile_t *cfp;
 
@@ -1297,13 +1317,13 @@
     parms.config_file = cfp;
     errmsg = ap_build_config(&parms, p, ptemp, &conftree);
     if (errmsg == NULL)
-	errmsg = ap_walk_config(conftree, &parms, s->lookup_defaults);
+	errmsg = ap_walk_config(conftree, &bad_directive, &parms, 
+                                s->lookup_defaults);
 
     if (errmsg != NULL) {
-	/* ### wrong line number. need to pull from ap_directive_t */
 	ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL,
                      "Syntax error on line %d of %s:",
-		     cfp->line_number, cfp->name);
+		     bad_directive->line_num, bad_directive->filename);
 	ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL, 
                      "%s", errmsg);
 	exit(1);
@@ -1348,14 +1368,14 @@
 
         if (status == APR_SUCCESS) {
 	    const char *errmsg;
-	    ap_directive_t *conftree;
+	    ap_directive_t *conftree, *bad_directive;
 
             dc = ap_create_per_dir_config(r->pool);
 
             parms.config_file = f;
             errmsg = ap_build_config(&parms, r->pool, r->pool, &conftree);
 	    if (errmsg == NULL)
-		errmsg = ap_walk_config(conftree, &parms, dc);
+		errmsg = ap_walk_config(conftree, &bad_directive, &parms, dc);
 
             ap_cfg_closefile(f);
 
Index: src/main/http_core.c
===================================================================
RCS file: /cvs/apache/apache-2.0/src/main/http_core.c,v
retrieving revision 1.62
diff -u -r1.62 http_core.c
--- http_core.c	2000/05/27 22:40:27	1.62
+++ http_core.c	2000/05/29 22:08:15
@@ -1288,7 +1288,8 @@
 }
 
 CORE_EXPORT_NONSTD(const char *) ap_limit_section(cmd_parms *cmd, void *dummy,
-						  const char *arg)
+						  const char *arg, 
+                                                  ap_directive_t **bad_directive)
 {
     const char *limited_methods = ap_getword(cmd->pool, &arg, '>');
     void *tog = cmd->cmd->cmd_data;
@@ -1321,7 +1322,8 @@
      */
     cmd->limited = tog ? ~limited : limited;
 
-    errmsg = ap_walk_config(cmd->directive->first_child, cmd, cmd->context);
+    errmsg = ap_walk_config(cmd->directive->first_child, bad_directive, cmd, 
+                            cmd->context);
 
     cmd->limited = -1;
 
@@ -1347,7 +1349,8 @@
 		      "> directive missing closing '>'", NULL);
 }
 
-static const char *dirsection(cmd_parms *cmd, void *dummy, const char *arg)
+static const char *dirsection(cmd_parms *cmd, void *dummy, const char *arg, 
+                              ap_directive_t **bad_directive)
 {
     const char *errmsg;
     char *endp = strrchr(arg, '>');
@@ -1389,7 +1392,8 @@
     conf = (core_dir_config *)ap_set_config_vectors(cmd, new_dir_conf,
 						    &core_module);
 
-    errmsg = ap_walk_config(cmd->directive->first_child, cmd, new_dir_conf);
+    errmsg = ap_walk_config(cmd->directive->first_child, bad_directive, cmd, 
+                            new_dir_conf);
     if (errmsg != NULL)
 	return errmsg;
 
@@ -1408,7 +1412,8 @@
     return NULL;
 }
 
-static const char *urlsection(cmd_parms *cmd, void *dummy, const char *arg)
+static const char *urlsection(cmd_parms *cmd, void *dummy, const char *arg,
+                              ap_directive_t **bad_directive)
 {
     const char *errmsg;
     char *endp = strrchr(arg, '>');
@@ -1447,7 +1452,8 @@
     conf = (core_dir_config *)ap_set_config_vectors(cmd, new_url_conf,
 						    &core_module);
 
-    errmsg = ap_walk_config(cmd->directive->first_child, cmd, new_url_conf);
+    errmsg = ap_walk_config(cmd->directive->first_child, bad_directive, cmd, 
+                            new_url_conf);
     if (errmsg != NULL)
 	return errmsg;
 
@@ -1468,8 +1474,8 @@
     return NULL;
 }
 
-static const char *filesection(cmd_parms *cmd, core_dir_config *c,
-			       const char *arg)
+static const char *filesection(cmd_parms *cmd, core_dir_config *c, 
+                               const char *arg, ap_directive_t **bad_directive)
 {
     const char *errmsg;
     char *endp = strrchr(arg, '>');
@@ -1514,7 +1520,8 @@
     conf = (core_dir_config *)ap_set_config_vectors(cmd, new_file_conf,
 						    &core_module);
 
-    errmsg = ap_walk_config(cmd->directive->first_child, cmd, new_file_conf);
+    errmsg = ap_walk_config(cmd->directive->first_child, bad_directive, cmd, 
+                            new_file_conf);
     if (errmsg != NULL)
 	return errmsg;
 
@@ -1620,7 +1627,8 @@
 
 /* httpd.conf commands... beginning with the <VirtualHost> business */
 
-static const char *virtualhost_section(cmd_parms *cmd, void *dummy, char *arg)
+static const char *virtualhost_section(cmd_parms *cmd, void *dummy, char *arg,
+                                       ap_directive_t **bad_directive)
 {
     server_rec *main_server = cmd->server, *s;
     const char *errmsg;
@@ -1660,7 +1668,7 @@
 
     cmd->server = s;
 
-    errmsg = ap_walk_config(cmd->directive->first_child, cmd,
+    errmsg = ap_walk_config(cmd->directive->first_child, bad_directive, cmd,
 			    s->lookup_defaults);
 
     cmd->server = main_server;
@@ -2165,33 +2173,33 @@
 
 /* Old access config file commands */
 
-{ "<Directory", dirsection, NULL, RSRC_CONF, RAW_ARGS,
+{ "<Directory", dirsection, NULL, RSRC_CONF, RAW_ARGS_D,
   "Container for directives affecting resources located in the specified "
   "directories" },
-{ "<Location", urlsection, NULL, RSRC_CONF, RAW_ARGS,
+{ "<Location", urlsection, NULL, RSRC_CONF, RAW_ARGS_D,
   "Container for directives affecting resources accessed through the "
   "specified URL paths" },
-{ "<VirtualHost", virtualhost_section, NULL, RSRC_CONF, RAW_ARGS,
+{ "<VirtualHost", virtualhost_section, NULL, RSRC_CONF, RAW_ARGS_D,
   "Container to map directives to a particular virtual host, takes one or "
   "more host addresses" },
-{ "<Files", filesection, NULL, OR_ALL, RAW_ARGS, "Container for directives "
+{ "<Files", filesection, NULL, OR_ALL, RAW_ARGS_D, "Container for directives "
   "affecting files matching specified patterns" },
-{ "<Limit", ap_limit_section, NULL, OR_ALL, RAW_ARGS, "Container for "
+{ "<Limit", ap_limit_section, NULL, OR_ALL, RAW_ARGS_D, "Container for "
   "authentication directives when accessed using specified HTTP methods" },
-{ "<LimitExcept", ap_limit_section, (void*)1, OR_ALL, RAW_ARGS,
+{ "<LimitExcept", ap_limit_section, (void*)1, OR_ALL, RAW_ARGS_D,
   "Container for authentication directives to be applied when any HTTP "
   "method other than those specified is used to access the resource" },
 { "<IfModule", start_ifmod, NULL, EXEC_ON_READ | OR_ALL, TAKE1,
   "Container for directives based on existance of specified modules" },
 { "<IfDefine", start_ifdefine, NULL, EXEC_ON_READ | OR_ALL, TAKE1,
   "Container for directives based on existance of command line defines" },
-{ "<DirectoryMatch", dirsection, (void*)1, RSRC_CONF, RAW_ARGS,
+{ "<DirectoryMatch", dirsection, (void*)1, RSRC_CONF, RAW_ARGS_D,
   "Container for directives affecting resources located in the "
   "specified directories" },
-{ "<LocationMatch", urlsection, (void*)1, RSRC_CONF, RAW_ARGS,
+{ "<LocationMatch", urlsection, (void*)1, RSRC_CONF, RAW_ARGS_D,
   "Container for directives affecting resources accessed through the "
   "specified URL paths" },
-{ "<FilesMatch", filesection, (void*)1, OR_ALL, RAW_ARGS,
+{ "<FilesMatch", filesection, (void*)1, OR_ALL, RAW_ARGS_D,
   "Container for directives affecting files matching specified patterns" },
 { "AuthType", ap_set_string_slot,
   (void*)XtOffsetOf(core_dir_config, ap_auth_type), OR_AUTHCFG, TAKE1,

-- 
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
     http://www.geocities.com/SiliconValley/Park/9289/
          Born in Roswell... married an alien...

Mime
View raw message