Great work, Geoff. But too big to swallow at once, may be will absorb it with time, but your descriptions sounds reasonable to me. I think you'd like to adjust/extend the docs while it's hot in your head ;)

I've just a few comments below:

Index: src/modules/perl/mod_perl.c
===================================================================
RCS file: /home/cvspublic/modperl-2.0/src/modules/perl/mod_perl.c,v
retrieving revision 1.206
diff -u -r1.206 mod_perl.c
--- src/modules/perl/mod_perl.c 10 Jan 2004 05:01:04 -0000 1.206
+++ src/modules/perl/mod_perl.c 6 Feb 2004 18:04:26 -0000
@@ -837,8 +837,30 @@
int modperl_response_handler(request_rec *r)
{
+ MP_dDCFG;
+
+#ifdef USE_ITHREADS
+ pTHX;
+ modperl_interp_t *interp;
+#endif
+
if (!strEQ(r->handler, "modperl")) {
return DECLINED;
+ }
+
+ /* default is -SetupEnv, add if PerlOption +SetupEnv */
+ if (MpDirSETUP_ENV(dcfg)) {
+ MP_dRCFG;
+
+#ifdef USE_ITHREADS
+ interp = modperl_interp_select(r, r->connection, r->server);
+ aTHX = interp->perl;
+ if (MpInterpPUTBACK(interp)) {
+ rcfg->interp = interp;
+ }
+#endif

What about modperl_interp_unselect? See modperl_response_handler_cgi.


also why modperl and perl-script have a duplication of the env handling, shouldn't it use the same code (probably moving into the _run handler).

But why did you introduce the interp select code? it's not bare bones handler any longer. can we get away w/o it?


> + modperl_env_request_populate(aTHX_ r);


Are you sure? 'modperl' doesn't do anything to %ENV unless +SetupEnv is on.

Index: t/modperl/cookie.t
===================================================================
RCS file: /home/cvspublic/modperl-2.0/t/modperl/cookie.t,v
retrieving revision 1.2
diff -u -r1.2 cookie.t
--- t/modperl/cookie.t  22 Nov 2003 07:38:48 -0000      1.2
+++ t/modperl/cookie.t  6 Feb 2004 18:04:28 -0000
@@ -6,8 +6,7 @@
 #
 # in this test we should be able get the cookie via %ENV,
 # since 'SetHandler perl-script' sets up mod_cgi env var. Moreover
-# adding 'PerlOptions +SetupEnv' adds them at the very first stage used
-# by mod_perl handlers, 'access' in this test. the last sub-test makes
+# calling $r->subprocess_env adds them on demand.  the last sub-test makes
 # sure, that mod_cgi env vars don't persist and are properly re-set at
 # the end of each request
 #

Index: t/response/TestModperl/cookie.pm
===================================================================
RCS file: /home/cvspublic/modperl-2.0/t/response/TestModperl/cookie.pm,v
retrieving revision 1.1
diff -u -r1.1 cookie.pm
--- t/response/TestModperl/cookie.pm 22 Sep 2003 23:34:45 -0000 1.1
+++ t/response/TestModperl/cookie.pm 6 Feb 2004 18:04:28 -0000
@@ -13,6 +13,9 @@
sub access {
my $r = shift;
+ # setup CGI variables early
+ $r->subprocess_env() if $r->args eq 'env';
+
my($key, $val) = cookie($r);
my $cookie_is_expected =
($r->args eq 'header' or $r->args eq 'env') ? 1 : 0;
@@ -48,7 +51,4 @@
PerlInitHandler Apache::TestHandler::same_interp_fixup
PerlAccessHandler TestModperl::cookie::access
PerlResponseHandler TestModperl::cookie
-# PerlOptions +SetupEnv is needed here, because we want the mod_cgi
-# env to be set at the access phase. without it, perl-script sets it
-# only for the response phase
-PerlOptions +SetupEnv
+PerlOptions -SetupEnv

why not using 'SetHandler modperl', instead of 'perl-script/-SetupEnv'?


In any case the comments in t/modperl/cookie.t are wrong with your .pm patch, because you have added -SetupEnv. I'm talking about:

>  # in this test we should be able get the cookie via %ENV,
>  # since 'SetHandler perl-script' sets up mod_cgi env var...

Also in modperl_env.c you had:

+    MP_dRCFG;
+    MP_dSCFG(r->server);
...
+    /* only once per-request */
+    if (MpReqPERL_SET_ENV_SRV(rcfg)) {
+        return;
+    }

sounds wasteful to me. It was outside the call originally. For some reason you've moved it in:

-        /* only happens once per-request */
-        if (MpDirSETUP_ENV(dcfg)) {
-            modperl_env_request_populate(aTHX_ r);
-        }
+        /* per-server PerlSetEnv and PerlPassEnv - only once per-request */
+        modperl_env_configure_request_srv(aTHX_ r);
+

same for modperl_env_configure_request_rec... oh may be I've got a bit lost...

__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]



Reply via email to