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 19:27:57 GMT
On Sat, 22 Apr 2000 rbb@covalent.net wrote:
> On Sat, 22 Apr 2000, Greg Stein wrote:
> > 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)
> 
> I have no problem with most of your mods, but this patch was getting
> unweildy as it was, so I wanted to commit it first.  :-)

Dude. No problem at all. That's why I put in the -0 ... you can check it
in if you'd like. Of course, it also says that I'd prefer if you made the
mods :-)

>...
> We didn't lose anything with this.  One of the nice things about working
> with Doug, is that he and I can discuss how things are done in the core
> and mod_perl.  :-)

Euh... but IIRC you can drop raw Perl code into the config file. We expect
Apache directives on each line. Things will gum up quickly if we see Perl.

> If anything, this makes the code easier for mod_perl.  Currently, mod_perl
> checks to see if there has been any perl code in the config, if not it
> starts a new interpreter.  And this means mod_perl's initializer hook
> (pre_config in 2.0) must be called before we read the config file.

Oh, yes. I do believe that we'll make things easier for mod_perl and such.
But I think we broke it temporarily :-).

> It is an easy change to read everything into the internal tree, pick out
> the modules, so EVERY module can have a pre_config hook again.  :-)  Then

Agreed!

> when we parse the tree, at some point, mod_perl's <Perl> handler will be
> called, and it will be passed the first directive in the perl
> section.

I thought that was raw Perl, not directives.

> Mod_perl just reads through that line of the config file, and
> re-creates the perl code, which is then passed to the already running
> interpreter.

Are you thinking that it would paste together "directive" and "args" to
recreate a line of Perl? eww :-)

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

Ah. Good call. I'll take a look at that. Probably use that for testing.

>...
> I'll make easy changes like this before the commit.  :-)  There is some
> cruft in this patch, because it was re-worked five times to get it
> working, and some things just weren't caught to be cleaned.  Trust me, the
> state of this code before the three read through's of the diff file was
> appaling.  :-)

Not a problem. I'll poke at whatever is left when you commit.

>...
> > 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.
> 
> That exit was there before.  I don't see this as a change in
> functionality, and I think this is definately where the code beongs, but
> I' willing to not make this change.  I say we commit and then you can
> modify it to your heart's content.

I know the exit was there, and the error reporting *may* belong there, but
both should not :-)

I'll fix it up after your commit.

>...
> > This should be an easy switch:
> > 
> >     config = ap_build_config(&parms, NULL);
> >     errmsg = ap_walk_config(config, &parms, s->lookup_defaults, 0);
> 
> Not quite that easy.  There is a part of me that wants to do it this way,
> but there is a bigger part of me that would like to see us build the tree
> here, and use the same tree to finish building the tree from the config
> file, and then parse it.  That is why I left this code alone.

With the easy change, we don't break the -c/-C stuff. Later, we can come
back and make the harder change.

>...
> > > @@ -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.
> 
> I'll fix the first problem before the commit.  The error message will have
> to wait until after this commit.

Okee dokee!

> Greg, thanks for the review.

You're quite welcome. Thanks for writing the code! :-)

Cheers,
-g

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


Mime
View raw message