On 3/8/15, Steve Holme <steve_holme_at_hotmail.com> wrote:
> On Fri, 6 Mar 2015, bch wrote:
>
>> Is the inline patch I posted above acceptable, or should it be
>> re-submitted as a collection of patches or something different ?
>
> I'm personally a Windows man so I would like attachments if possible -
> trying to apply an inline git patch is a nightmare (just doesn't work due to
> line endings and Outlook / Hotmail messing about with the formatting!)
>
> I know others here prefer inline so I would always recommend sending in both
> ;-)

very fair...

> From my brief review I would recommend the following naming as they are more
> in keepingwith the existing SSL informational items:
>
>> + CURLINFO_NEGOTIATED_SSL = CURLINFO_LONG + 44,
>
> CURLINFO_SSL_NEGOTIATED_VERSION

Useful...

> ...and for consistency:
>
>> + info->negotiated_ssl = -1L;

ditto

> info->ssl_negotiated_version
>
> A couple of small nits:
>
> * I would recommend removing such lines as "/* bch ref -- NEGOTIATED_SSL
> info here (?) */"

This wouldn't make it into anything I'd submit -- it's note-taking
for myself if the approach I put forward was deemed reasonable.

> * Open braces after a function declaration should be on the following line
> not as per conditional code like you appear to have done with
> "set_ssl_version_long(SSL *ssl, struct connectdata *conn) {"

Noted...

> * I would also recommend that the return type be on the beginning rather
> than on a separate line - I appreciate we have a mix of this at present :(

Noted...

If the approach in general looks sane, I'll make the formatting
adjustments and start fleshing out other SSL engines as well. Thanks
for the feedback.

> Kind Regards
>
> Steve

Cheers,

-bch
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2015-03-08