httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] Config modules step 1
Date Wed, 12 Apr 2000 00:19:32 GMT
On Tue, 11 Apr 2000 rbb@covalent.net wrote:
>...
> +    static ap_directive_t *current = NULL;

It would be nice (in the future) to not use a static. But quite fine for
now :-)

Below I note that a current_parent is also needed.

>...
> +
> +    newdir = ap_pcalloc(parms->pool, sizeof(ap_directive_t));
> +    newdir->line_num = parms->config_file->line_number;
> +    newdir->directive = ap_pstrdup(parms->pool, cmd_name);
> +    newdir->args = ap_pstrdup(parms->pool, args);
> +
> +    if (cmd_name[0] == '<') {
> +        current = ap_add_node(current, newdir, 1);
> +    }
> +    else {
> +        current = ap_add_node(current, newdir, 0);
> +    }
> +    newdir = NULL;
> +    if (strncmp(cmd_name, "</", 2) == 0) {
> +        current = current->parent;
> +    }

The cmd_name[0] will erroneously catch the "</foo>" case.

Also, this means we have nodes in the tree that correspond to the "close"
case. IMO, we don't want close nodes -- the tree itself describes the
open/close semantics and close nodes have no specific data.

>...
> #ifndef AP_CONFTREE_H
> #define AP_CONFTREE_H
> 
> #include "apr_lib.h"

Not needed.

> typedef struct ap_directive_t {
>     char *directive;
>     char *args;

"const char *" would be preferable.

>...
> 
> #define CORE_PRIVATE
> #include "util_cfgtree.h"
> 
> ap_directive_t *conf_tree;
> 
> ap_directive_t *ap_add_node(ap_directive_t *current, ap_directive_t *toadd, int child)
> {
>     if (current == NULL) {
>         conf_tree = toadd;
>         return conf_tree;
>     }
>     else if (child == 1){

No need for "else" since the primary condition has a "return"

>         current->first_child = toadd;
>         toadd->parent = current;
>         toadd->first_child = toadd->next = NULL;
>         return toadd;

The structure of this plus the caller implies that you'll end up with a
node tree like:

  Top-level directive
  Top-level directive
      <Directory /foo>
      Dir-level directive
      Dir-level directive
      </Directory>
  Top-level directive

A more intuitive tree would look like:

  Top-level directive
  Top-level directive
  <Directory /foo>
      Dir-level directive
      Dir-level directive
  Top-level directive

To achieve this, record the current_parent. All nodes are added into that
parent. When a <Directory> occurs, the current_parent is shifted "down" to
the <Directory> node.

Note that "current" is still necessary, so that you can add at the "tail"
of the linked list of sibling nodes.


Let's see... the ap_add_node() would look kinda like:

ap_directive_t *ap_add_node(ap_directive_t **parent,
                            ap_directive_t *current,
                            ap_directive_t *toadd, int child)
{
    if (current == NULL) {
       /* we just started a new parent */
       if (*parent == NULL) {
          /* no parent, no current: root of tree */
          cfg_tree = toadd;
       } else {
          (*parent)->first_child = toadd;
       }
       return toadd;
    }
    current->next = toadd;
    toadd->parent = *parent;
    if (child) {
      /* switch parents, navigate into child */
      *parent = toadd;
      return NULL;
    }
    return toadd;
}

The return value is the new current node. Navigating "up" involves
changing the current_parent and current (pointing to the same node).

Note that top-level nodes have parent==NULL. If that is not desired, we
can always insert a dummy placeholder node to operate as the parent.

Cheers,
-g

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



Mime
View raw message