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??

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
(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 ;-)
* 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?
* 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 ;-)
* 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

Kind Regards

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