perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philippe M. Chiasson" <go...@ectoplasm.org>
Subject Re: [Patch mp2] PerlConfigRequire & PerlPostConfigRequire ( was: a new directive: PerlStartupFile? )
Date Sat, 11 Dec 2004 18:25:27 GMT
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

Mime
View raw message