On 11/19/2013 09:44 PM, Steve Holme wrote:
> Hi Christian,
>
> On Tue, 19 Nov 2013, Steve Holme wrote:
>
>>> Well, my first patch had the application own the memory.
>>> Daniel didn't like that idea as too error-prone. So go figure.
>>
>> I'll have a look at your original patch tomorrow, to see if we
>> are talking about the same thing, but I thought (off the top
>> of head) that option 2 that Daniel mentioned was different
>> to your original implementation.
>
> I've had a look at your original patch and re-read Daniel's reply a few
> times. I think his main gripe (and obviously I can't speak for him) was with
> the way the gptr union was being passed in:
>
> union {
> struct curl_tlsinfo *tlsinfo;
> struct curl_slist *to_slist;
> } gptr;
>
> curl_easy_getinfo (s5r->curl,
> CURLINFO_TLS_SESSION,
> &gptr))
>
> ...rather than with the memory being owned by the application.
>
> I understand what you were trying to achieve with the original patch and
> that it kind of fits in with the CURLINFO_CERTINFO slist paradigm, however,
> if you take a look at the certinfo.c example, it passes in ptr.to_info
> rather than the whole union - I guess the benefit here is that you don't
> need to cast and some might argument that this is easier to understand (I'm
> not convinced!), and under some compilers there might be a benefit??

I don't know of any benefits under some compilers; I personally wouldn't
use
this style, but I was trying to follow the style I was seeing in the code.

> union {
> struct curl_slist *to_info;
> struct curl_certinfo *to_certinfo;
> } ptr;
>
> ptr.to_info = NULL;
> curl_easy_getinfo(curl, CURLINFO_CERTINFO, &ptr.to_info);
>
> Speaking of how CURLINFO_CERTINFO does things, it would seem that it returns
> a pointer to the libcurl owned memory, rather than create a copy, and whilst
> as you know I'm not a fan of this practice, your patch is at least in
> keeping with how that works and I like consistency - which was kind of the
> point of my original email in case I add a capability_info structure at some
> point in the future ;-)
>
> I'm happy to start pushing your work into the upcoming v7.34.0 release

Which version? The latest patch, or an earlier one?

> (unless anyone has any objections with that) as I appreciate I have held
> things up for a couple of days, however, I do have a few questions about
> your patch:
>
> * Are you able to write up an example that uses this rather than place the
> example in the commit comments? A little like certinfo.c ;-)

Sure, but please let me know which version / variant of the implementation
you'll use so I won't write example for the wrong one.

> * What happens if, as Dan mentioned, the SSL context isn't initialised /
> created? Will the calling application simply get an invalid structure or are
> we able to have curl_easy_getinfo() fail?

There is an 'enum' value for 'invalid', and the void * would be NULL.
As I figured people might not check the return value of curl_easy_getinfo,
but would HAVE to do a switch on the enum, I think that's the most robust
variant we can offer.

> * In your curl_easy_getinfo.3 modification was the intention to have the two
> paragraphs separate? If so the web page generator will need a blank line. I
> can add this but I will need to know ;-)

Whatever you like better, I have no strong feelings either way.

> * More for future reference (as I can edit them by hand before I integrate
> your work) but the commit comments are not in keeping with how we do things
> - Please see:
> http://curl.haxx.se/dev/contribute.html#Write_good_commit_messages

Ack.

-Christian
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-11-20