httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: Config tree code finished.
Date Sat, 22 Apr 2000 08:34:25 GMT
On Fri, 21 Apr 2000 rbb@covalent.net wrote:
>...
> 1)  Apply this patch

+1  (only with the mods noted below; -0 as is, and I'll make the mods)

> 2)  Remove all of the config tree code.

-1

> I am +0 for either one.  I wrote this code, because the ending goal was to
> get module writers to define directives differently.  When I started, the
> first step was to create configuration modules.  That idea has lost all
> backing from everybody.

Everybody!!?!  Don't be so extreme. :-)

>...

I've got a few minor nits noted below. There are some add'l changes that
I'd like to see, but I'll code/deal with those after you check in the
code.

Note that we saw a number of agreements on using the config tree. It was
the "loadable module" or "selectable config syntax" thing that thread
people. IMO, this patch neither adds nor removes functionality -- it is
simply a change in operation. Hopefully, it shouldn't be a big discussion
item. (read: just apply the sucker for commit-then-review :-)

Ryan: I like this. During the review of this patch, I realized just what
the hell you were up against. Ewww... :-)  This patch helps to untwist it.

I have realized one thing: we lost a hook for modules to parse the config
file themselves. Specifically, in the old scheme, when their command
handler was called, they could grab cmd_parms->config_file and read lines
for a while. I presume this is how mod_perl manages to get raw Perl code
into an Apache config file. We can provide the *same* functionality in
2.0, but modules that hack the config file like this will need to make a
few changes for 2.0 compatibility. Basically: they can parse the file at
read/build time, drop a context pointer into ap_directive_t, and have that
context passed during command processing. Note that the hook into the
process will be specific to the Apache-style config-reader (as it is
today).

Q: are there any other modules that whack into the config file reading
process like this? (none in the standard distro)

Final note before code review: I am *REALLY* glad to see this happen after
looking through some of the config processing during this review. Man,
that stuff is nasty. We really need to apply this patch to create a sane
config reading, parsing, and handling mechanism. This will also help to
create a better system for the third party modules that want to tap into
the config process.


>...
> +CORE_EXPORT(void) ap_build_config_sub(cmd_parms *parms, const char *l, ap_directive_t
**current, ap_directive_t **curr_parent)
>...
>      newdir = NULL;
> +}

That assignment isn't needed :-)

> +API_EXPORT(const char *)ap_walk_config_sub(ap_directive_t *current, cmd_parms *parms,
void *config)
> +{
> +    void *oldconfig;
> +    const command_rec *cmd;
> +    module *mod = top_module;
> +    const char *retval;
>  
>      oldconfig = parms->context;
>      parms->context = config;
>      do {
> -	if (!(cmd = ap_find_command_in_modules(cmd_name, &mod))) {
> +	if (!(cmd = ap_find_command_in_modules(current->directive, &mod))) {
>              errno = EINVAL;

I see this errno was from before, but it should just disappear.

> -            return ap_pstrcat(parms->pool, "Invalid command '", cmd_name,
> +            retval = ap_pstrcat(parms->pool, "Invalid command '", 
> +                           current->directive,
>                             "', perhaps mis-spelled or defined by a module "
>                             "not included in the server configuration", NULL);
>  	}
>  	else {
>  	    void *mconfig = ap_set_config_vectors(parms,config, mod);
>  
> -	    retval = invoke_cmd(cmd, parms, mconfig, args);
> +	    retval = invoke_cmd(cmd, parms, mconfig, current->args);
>  	    mod = mod->next;	/* Next time around, skip this one */
>  	}
>      } while (retval && !strcmp(retval, DECLINE_CMD));
>      parms->context = oldconfig;
>  
> -    return retval;
> +    if (retval) {
> +	ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL,
> +                     "Syntax error on line %d of %s:",
> +		     parms->config_file->line_number, parms->config_file->name);
> +	ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL, 
> +                     "%s", retval);
> +	exit(1);
> +    }
>  }

You lost a "return" in here.

Moving this error message here (from ap_process_resource_config) is a
mistake. It is a change in functionality and that exit(1) definitely
should not be present. I would recommend moving it back and dealing with
error handling in a second pass.

> -API_EXPORT(const char *) ap_srm_command_loop(cmd_parms *parms, void *config)
> +API_EXPORT(const char *)ap_walk_config(ap_directive_t *conftree, cmd_parms *parms, void
*config, int container)
>  {
> +    static ap_directive_t *current;
> +
> +    if (conftree != NULL) {
> +        current = conftree;
> +    }
> +    if (container && current->first_child) {
> +        current = current->first_child;
> +    }
> +
> +    while (current != NULL) {
> +        /* actually parse the command and execute the correct function */
> +        ap_walk_config_sub(current, parms, config);

Need to check the return value from this and exit.

> +
> +        if (current->next == NULL) {
> +            current = current->parent;
> +            break;
> +        } else {
> +            current = current->next;
> +            continue;
> +        }
> +    }
> +}

Missing return value. Should match the error message handling of the old
ap_srm_command_loop().

> +API_EXPORT(ap_directive_t *) ap_build_config(cmd_parms *parms, ap_directive_t *current)
> +{
> +    ap_directive_t *curr_parent = NULL;
> +    ap_directive_t *cfg_root;
>      char l[MAX_STRING_LEN];
>  
>      while (!(ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))) {
> -	const char *errmsg = ap_handle_command(parms, config, l);
> -        if (errmsg) {
> -	    return errmsg;
> +	ap_build_config_sub(parms, l, &current, &curr_parent);
> +        if (current != NULL) {
> +            cfg_root = current;
> +            break;
>  	}
>      }
>  
> -    return NULL;
> +    while (!(ap_cfg_getline(l, MAX_STRING_LEN, parms->config_file))) {
> +	ap_build_config_sub(parms, l, &current, &curr_parent);
> +    }
> +
> +    return cfg_root;
>  }

These loops can be integrated if you initialize cfg_root to NULL and then
include the following after the ap_build_config_sub() call:

    if (cfg_root == NULL && current != NULL)
        cfg_root = current;

> @@ -1039,8 +1082,8 @@
>      parms.config_file = ap_pcfg_open_custom(p, "-c/-C directives",
>                                           &arr_parms, NULL,
>                                           arr_elts_getstr, arr_elts_close);
> -
> -    errmsg = ap_srm_command_loop(&parms, s->lookup_defaults);
> +/*
> +    errmsg = ap_build_config(&parms, s->lookup_defaults);

This should be an easy switch:

    config = ap_build_config(&parms, NULL);
    errmsg = ap_walk_config(config, &parms, s->lookup_defaults, 0);

>  
>      if (errmsg) {
>          ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL,
> @@ -1049,13 +1092,14 @@
>      }
>  
>      ap_cfg_closefile(parms.config_file);
> +*/
>  }
>  
>  void ap_process_resource_config(server_rec *s, const char *fname, ap_pool_t *p, ap_pool_t
*ptemp)
>  {
> -    const char *errmsg;
>...
> @@ -1081,16 +1125,8 @@
>  	exit(1);
>      }
>  
> -    errmsg = ap_srm_command_loop(&parms, s->lookup_defaults);
> -
> -    if (errmsg) {
> -	ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL,
> -                     "Syntax error on line %d of %s:",
> -		     parms.config_file->line_number, parms.config_file->name);
> -	ap_log_error(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, NULL, 
> -                     "%s", errmsg);
> -	exit(1);
> -    }

As mentioned: this should stick around. The errmsg should be set to the
return value from ap_walk_config().

> +    current = ap_build_config(&parms, current);

current == NULL here. It is clearer to pass NULL.

>...
> @@ -1101,12 +1137,13 @@
>  {
>      configfile_t *f = NULL;
>      cmd_parms parms;
> -    const char *errmsg;
> +    const char *errmsg = NULL;
>      char *filename = NULL;
>      const struct htaccess_result *cache;
>      struct htaccess_result *new;
>      void *dc = NULL;
>      ap_status_t status;
> +    ap_directive_t *htaccess_tree = NULL;
>  
>  /* firstly, search cache */
>      for (cache = r->htaccess; cache != NULL; cache = cache->next)
> @@ -1136,7 +1173,8 @@
>  
>              parms.config_file = f;
>  
> -            errmsg = ap_srm_command_loop(&parms, dc);
> +            htaccess_tree = ap_build_config(&parms, htaccess_tree);

Again: htaccess_tree is NULL and it would be clearer to just pass NULL.

>...
> @@ -1147,11 +1185,9 @@
>              }
>              *result = dc;
>              break;
> -        } else {
> -            ap_status_t cerr = ap_canonical_error(status);
> -
> -            if (cerr != APR_ENOENT && cerr != APR_ENOTDIR) {
> -                ap_log_rerror(APLOG_MARK, APLOG_CRIT, status, r,
> +        }
> +        else if (status != APR_ENOENT && status != APR_ENOTDIR) {
> +            ap_log_rerror(APLOG_MARK, APLOG_CRIT, errno, r,

You lost the canonical stuff, and picked up an "errno" rather than
"status".

>...
> Index: main/http_core.c
> ===================================================================
> RCS file: /home/cvs/apache-2.0/src/main/http_core.c,v
> retrieving revision 1.48
> diff -u -d -b -w -u -r1.48 http_core.c
> --- main/http_core.c	2000/04/14 15:58:55	1.48
> +++ main/http_core.c	2000/04/22 01:57:19
> @@ -1383,7 +1383,7 @@
>  	return ap_pstrcat(cmd->pool, "Expected ", cmd->end_token, " but saw ",
>  			  cmd->cmd->name, NULL);
>      }
> -    return cmd->end_token;
> +    return NULL;
>  }

Boy, the old code was twisted. But this change is quite correct!

We'll get the section open/close testing in there properly (during the
build). When that occurs, then we can clean out some of this old cruft.
*Definitely* for a later pass rather than attempting to include in this
patch. :-)

>...
> @@ -1637,23 +1616,9 @@
>      found = ap_find_linked_module(arg);
>  
>      if ((!not && found) || (not && !found)) {
> +        ap_walk_config(NULL, cmd, cmd->server->lookup_defaults, 1);

This call and all other calls to ap_walk_config() should get the return
value (errmsg) and return it if non-NULL.

Modules are more than capable of raising errors in their command handling.
We need to propagate those errors.

>          return NULL;
>      }
> -
> -    while (nest && !(ap_cfg_getline(l, MAX_STRING_LEN, cmd->config_file)))
{
> -        if (!strncasecmp(l, "<IfModule", 9)) {
> -	    nest++;
> -	}
> -	if (!strcasecmp(l, "</IfModule>")) {
> -	  nest--;
> -	}
> -    }
> -
> -    if (nest) {
> -	cmd->end_token = end_ifmodule_section;
> -	return missing_endsection(cmd, nest);
> -    }
> -    return NULL;
>  }

Lost a return value here.

>  API_EXPORT(int) ap_exists_config_define(char *name)
> @@ -1698,22 +1663,9 @@
>      defined = ap_exists_config_define(arg);
>  
>      if ((!not && defined) || (not && !defined)) {
> +        ap_walk_config(NULL, cmd, dummy, 1);
>  	return NULL;
>      }
> -
> -    while (nest && !(ap_cfg_getline(l, MAX_STRING_LEN, cmd->config_file)))
{
> -        if (!strncasecmp(l, "<IfDefine", 9)) {
> -	    nest++;
> -	}
> -	if (!strcasecmp(l, "</IfDefine>")) {
> -	    nest--;
> -	}
> -    }
> -    if (nest) {
> -	cmd->end_token = end_ifdefine_section;
> -	return missing_endsection(cmd, nest);
> -    }
> -    return NULL;
>  }

Lost another return value. I'd suggest -Wall on your compilations :-)

> @@ -1760,11 +1712,8 @@
>      old_end_token = cmd->end_token;
>      cmd->end_token = end_virtualhost_section;
>      cmd->server = s;
> -    errmsg = ap_srm_command_loop(cmd, s->lookup_defaults);
> -    cmd->server = main_server;
> -    if (errmsg == NULL) {
> -	errmsg = missing_endsection(cmd, 1);
> -    }
> +
> +    ap_walk_config(NULL, cmd, s->lookup_defaults, 1);

You lost the assignment to cmd->server. And need to check the errmsg.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/


Mime
View raw message