Re: [Patch mp2] PerlConfigRequire & PerlPostConfigRequire ( was: a new directive: PerlStartupFile? )

[Philippe M. Chiasson]

Stas Bekman wrote:

Philippe M. Chiasson wrote:

After some talk about exactly what to call these directive, here is a patch. It's late, so it doesn't include
docs or tests, but it should give a good idea of what's required to get this working.

PerlConfigRequire is basically just PerlRequire with an immediate perl startup
PerlPostConfigRequire delays requiring the files up to the last possible moment, before interpreters begin
to get cloned, at the begining of mp's post_config phase.

Feedback most welcome!

gozer++

comments below

Index: src/modules/perl/mod_perl.c
===================================================================
@@ -646,6 +665,11 @@
     MP_dSCFG(s);
     dTHXa(scfg->mip->parent->perl);
 #endif
+    +    if (!modperl_post_config_require(s, pconf)) {
+        exit(1);
+    }

shouldn't it return an error (code) here, rather than exit?

Not really, since it takes care of logging the problem internally, the only important
thing is to be able to propagate failure up and stop apache's startup.

also I find modperl_post_config_require not very intuitive, but I can't think of anything better. I guess it's OK, since it's only local. may be modperl_post_config_require_apply. dunno.

Yeah, I had the same naming problem, modperl_post_config_require_apply is
only a slightly better name, IMO.

Index: src/modules/perl/modperl_config.c
===================================================================
--- src/modules/perl/modperl_config.c (revision 111578)
+++ src/modules/perl/modperl_config.c (working copy)
@@ -155,6 +155,7 @@
scfg->PerlModule = apr_array_make(p, 2, sizeof(char *));
scfg->PerlRequire = apr_array_make(p, 2, sizeof(char *));
+ scfg->PerlPostConfigRequire = apr_array_make(p, 2, sizeof(char *));

s/sizeof(char *)/sizeof(modperl_require_file_t *)/

probably 1 table entry should be enough here.

I was just following what seems to be a convention to go for 2.

Index: src/modules/perl/modperl_types.h
===================================================================
--- src/modules/perl/modperl_types.h (revision 111578)
+++ src/modules/perl/modperl_types.h (working copy)
@@ -65,6 +65,11 @@
};
typedef struct {
+ const char *file;
+ PerlInterpreter *perl; +} modperl_require_file_t;

shouldn't *perl be ifdef'ed by

#ifdef USE_ITHREADS
    PerlInterpreter *perl;
#endif

You don't have aTHX under non-threaded perl.

Same of course goes to the population of this variable and its usage (see APR__Pool.h for a similar case).

Index: t/conf/post_config_startup.pl
===================================================================
--- t/conf/post_config_startup.pl (revision 111578)
+++ t/conf/post_config_startup.pl (working copy)
@@ -148,3 +148,5 @@
}
1;
+
+die "blargh";

what's this die for?

That's the kind of leftover code that gets into patches submitted past 
midnight ;-)

Index: t/conf/extra.conf.in
===================================================================
--- t/conf/extra.conf.in (revision 111578)
+++ t/conf/extra.conf.in (working copy)
@@ -1,6 +1,8 @@
# needed to test $r->psignature
ServerSignature On
+PerlPostConfigRequire @ServerRoot@/conf/post_config_startup.pl
+
# The following tests require more than one interpreter during the
# same request:
#
Index: todo/release

Also we discussed that we need to add a similar PerlPostConfigRequire for t/vhost and add a test similar to t/hooks/TestHooks/startup.pm and modperl_extra.pl:test_hooks_startup();

Yup, a revised patch including this will make it's way to the mailing-list.

--------------------------------------------------------------------------------
Philippe M. Chiasson m/gozer\@(apache|cpan|ectoplasm)\.org/ GPG KeyID : 88C3A5A5 http://gozer.ectoplasm.org/ F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3A5A5

begin:vcard
fn:Philippe M. Chiasson
n:Chiasson;Philippe M.
email;internet:[EMAIL PROTECTED]
x-mozilla-html:FALSE
version:2.1
end:vcard

signature.asc
Description: OpenPGP digital signature