httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Sylvester <peter.sylves...@edelweb.fr>
Subject Re: [Fwd: backport and enhancement of patch 724717]
Date Thu, 10 Sep 2009 14:20:45 GMT
Joe Orton wrote:
> On Wed, Sep 09, 2009 at 10:22:28PM +0200, Peter Sylvester wrote:
>   
>> The patch for 724717 moves some logic from ssl_engine_kernel into
>> ssl__engine_vars and simplifies the code (and enhances it btw).
>> Can this code be backported to the 2.2.x version
>>     
>
> Have you done any testing on that?  I hadn't done much beyond running 
> the test suite, IIRC.
>   
I think you want to know whether the code does not extract
differently than the other one? I.e. the subset is identical
including all errors? :-)
Can you point me to the test suites? TIA

>> The enhancement is to add the "_n" suffix for multiple occurences
>> of attributs which seems good. in addition it concentrates logic
>> at one place ...
>>
>> ... almost. Wouldn't it be better to move the for loop in the
>> following snippet of ssl_engine_kernel (and the ssl_hoolFixup_vars
>> table also inside the the routine above (changing maybe its name).
>>     
>
> That separation was deliberate - the modssl_var_extract_dns() function 
> purely handles the DN extraction - the rest of the variables are 
> unrelated to the DN handling.  I don't see that's a big issue.
>   
I understand that modssl_var_extract_dns is for the dns.
That's why I mentioned another name like let's say:

   modssl_var_extract.

Thesis/antithesis:

If one wants to add whatever new variable, it requires
modification of kernel and vars. you need the code in
vars, and piloting via the table in kernel. One can argue
in favour of that or the contrary. 

I'd prefer to have a code that calls just one function in
vars so instead of

  if (dc->nOptions & SSL_OPT_STDENVVARS) {
      modssl_var_extract_dns(env, sslconn->ssl, r->pool);

      for (i = 0; ssl_hook_Fixup_vars[i]; i++) {
          var = (char *)ssl_hook_Fixup_vars[i];
          val = ssl_var_lookup(r->pool, r->server, r->connection, r, var);
          if (!strIsEmpty(val)) {
              apr_table_setn(env, var, val);
          }
      }
  }

one would have for example

  if (dc->nOptions & SSL_OPT_STDENVVARS)
      modssl_var_extract(env, sslconn->ssl, r->pool);

or even

   modssl_var_extract(env, sslconn->ssl, r->pool, dc->nOptions); 

and let the function decide what to do.
(or with dc or whatever parameter that would be the most convenient)

regards







Mime
View raw message