You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Eissing <st...@greenbytes.de> on 2018/06/04 14:07:57 UTC
Re: SSLPolicy code questions/backport review
Thanks for the review! I will take this and clean up the code, mod_ssl certainly deserves it.
> Am 23.05.2018 um 09:51 schrieb Joe Orton <jo...@redhat.com>:
>
> Easier to do here than dump in STATUS; looking at reviewing the 2.4.x
> backport:
>
> https://svn.apache.org/repos/asf/httpd/httpd/patches/2.4.x/ssl-policy.patch
>
> 0. Overall looks good, I like the way this has been done!
>
> 1. Is there a reason why we need SSLPolicyRec with (name, sc) members
> rather than having a hash directly of SSLSrvConfigRec *? The only time
> ->name is used seems to be when creating the hash in add_policy().
> [Reading back through commits, I guess this was a legacy of having ->dc
> in there too]
>
> 2. Storing the policies hash off pconf userdata seems surprising, is
> there a reason that's done rather than putting it directly in
> SSLModConfigRec?
>
> 3. get_policy_names() seems be only now called with create=1, and hence
> the same is true for get_policies(), so there are some redundant paths +
> redundant arguments here which could be removed?
>
> 4. There is a bunch of legacy from the now-removed SSLPolicyDefine
> AFAICT still included on trunk (not in the 2.4 patch); there is a
> reference added in the docs in the 2.4 patch though. e.g.
>
> BOOL ssl_config_global_isfixed(SSLModConfigRec *mc)
> {
> - return mc->bFixed;
> + return mc && mc->bFixed;
>
> is I think related and in trunk all the ssl_cmd_* handling of mc==NULL
> case (again not in the backport).
>
> 5. Very minor, please don't reformat the code now, but httpd code style
> has case X: statements lined up with switch rather than indented.
>
> Regards, Joe