httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: svn commit: r798359 - in /httpd/httpd/branches/2.2.x: modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_engine_vars.c modules/ssl/ssl_util_ssl.c support/ab.c
Date Wed, 29 Jul 2009 17:17:32 GMT
Guenter Knauf wrote:
> Hi Peter,
> Peter Sylvester schrieb:
>> A little nit in ssl_engine_init.c:
>> instead of
>>
>> -        SSL_CTX_set_client_CA_list(ctx, (STACK *)ca_list);
>> +        SSL_CTX_set_client_CA_list(ctx, (STACK_OF(X509_NAME) *)ca_list);
>>
>> I think I'd prefer
>> +        SSL_CTX_set_client_CA_list(ctx, ca_list);
> but this is not part of the proposed patch?
> http://people.apache.org/~fuankg/diffs/openssl-1.x-2.2.x.diff
> and its already fixed in HEAD:
> http://svn.apache.org/viewvc?view=rev&revision=798274
> 
>> and a few lines later instead of
>>
>> ca_list = (STACK_OF(X509_NAME) *)SSL_CTX_get_client_CA_list(ctx);
>>
>>   it should be
>> ca_list = SSL_CTX_get_client_CA_list(ctx);
> this is also not part of the patch in question nor is it required.

For trunk, it should be committed.  Casts are generally signs of a dumb
compiler or poor coding design and must be avoided.

> Instead it is another cleanup which should go the usual way = apply in
> HEAD, propose for backport. Please lets separate these things - the
> bigger we make the one 2.2.x backport patch the lesser the other
> developers are in the mood to review it.

I'm generally -1 for stylistic cleanup backports.  Although it might bring
trunk and the 2.2 branch into closer alignment, the fact is that they
diverge significantly enough not to sweat that.  If an actual compilation or
cast/arg flaw is discovered on trunk, of course that should be backported.

> Please lets focus on the proposed 2.2.x patch only at the moment, and
> make sure that it compiles warning-free with 0.9.7, 0.9.8 and 1.0.0.

+1

> now that the patch was already in 2.2.x trunk it should be very easy to
> apply it, just do a:
> svn merge -r798358:798359
> https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x
> and you should get all hunks in ... :)

Almost compiles clean in all three on Win32 32 bit x86 architectures.

For 1.0.0beta3;

C:\local0\asf\build\httpd-2.2\modules\ssl\ssl_engine_init.c(1175) : warning
C4028: formal parameter 1 different from declaration
C:\local0\asf\build\httpd-2.2\modules\ssl\ssl_engine_init.c(1175) : warning
C4028: formal parameter 2 different from declaration

Apparently this is bogus;

static int ssl_init_FindCAList_X509NameCmp(X509_NAME **a, X509_NAME **b)

It seems this solves the emits...

static int ssl_init_FindCAList_X509NameCmp(const X509_NAME * const*a,
                                           const X509_NAME * const*b)

So I'm +1'ing your STATUS entry, and recommending you also fix the constness
flaw above.  Well done :)

Pity that ml.exe support was stripped (ms\do_masm), it would be a shame
for most VC users to drop asm optimizations.



Mime
View raw message