On Mon, 3 May 2010, Hoi-Ho Chan wrote:

> Here is the patch for PolarSSL against 7.20.1. Please feel free to take a
> look and see if I have done it right :).

Thanks a lot! I've pushed your patch with a series of edits. My initial
comments follow:

o #includes of polarssl headers should be done with <> and not "" around the
   file names.

o The function polarssl_debug() shouldn't be declared unless POLARSSL_DEBUG
   is defined, as it causes warnings otherwise (unused static function).

   In general, you should consider running configure --enable-debug and build
   everything as then you would've seen a couple of more warnings.

o Using --enable-debug in your build, and then running the test suite had
   another effect:

   $ ./runtests.pl 1
   [...]
   ** MEMORY FAILURE
   Close without open: FD polarssl.c:346 sclose(5)
   Close without open: FD url.c:2476 sclose(5)

   This particular bug was very easy to fix, but it shows that you could easily
   test this a bit more.

o I'm very puzzled by the differences between HAVE_POLARSSL_GPL and without
   it, as it seems the API and all sorts of stuff differences. Is this
   documented anywhere? In fact, I'm a bit sad to see that polarssl seems to be
   even worse than the other SSL libraries in the docs department and I fail to
   find *ANY* docs for *ANY* function calls? Am I just not looking at the right
   places? The web site seems to indicate that polarssl is available as GPL or
   proprietary licenced but I can't see anything about them not having the same
   API or functionality?

o If HAVE_POLARSSL_GPL is not defined, what does x509parse_cert_info() return?
   Isn't that allocated memory that need to be freed somehow and thus your
   patch introduces a memory leak?

o Regarding the weird work-around for a bug that is commented as:
   /* FIXME: PolarSSL has a bug where we need to import it twice */
   Won't that introduce a memory leak once the function works the first time?
   Shouldn't the conditional check a specific polarssl version for which to use
   this kludge?

o Speaking of version, is there really no way (build time or run time) to
   figure out what PolarSSL version that is used?

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html