[asterisk-dev] [Code Review]: func_curl: make it report errors like "server certificate verification failed" instead of returning silently

[wdoekes]

> On March 19, 2013, 10:31 a.m., Tilghman Lesher wrote:
> > /branches/1.8/funcs/func_curl.c, line 634
> > <https://reviewboard.asterisk.org/r/2403/diff/2/?file=34835#file34835line634>
> >
> >     Given that this is a static buffer, I think stylistically, it would be better for it to be declared at the top of the function, rather than in a separate block.  I think we generally only do the separate block under very special circumstances, and this isn't one of them.

I can do that.


> On March 19, 2013, 10:31 a.m., Tilghman Lesher wrote:
> > /branches/1.8/funcs/func_curl.c, line 643
> > <https://reviewboard.asterisk.org/r/2403/diff/2/?file=34835#file34835line643>
> >
> >     It's not clear from the documented libcurl API that this will do what you expect.  The API suggests that you should instead be using CURLOPT_STDERR to turn off the buffer.
> 
> Tilghman Lesher wrote:
>     As an addendum, I realize that you tested this, but given that the API is not documented that way, it could be changed in the future, and we'd be wrong for assuming that it would always work this way.  I think I'd prefer if we used the API as documented, rather than making assumptions.

While I agree that one generally shouldn't be making such assumptions, I don't see how CURLOPT_STDERR helps me in this case.

The docs[1] explicitly state that I can change it. Between the lines I read that I can clear it:

   libcurl will need it until you call curl_easy_cleanup(3) or you set the same option again to use a different pointer.

The mailing list[2] and a corresponding commit[3] also suggests that it is valid:

  Subject: typecheck: allow NULL to unset CURLOPT_ERRORBUFFER
  From: Daniel Stenberg <daniel_at_haxx.se> 
  Thanks, pushed!


[1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTERRORBUFFER
[2] http://curl.haxx.se/mail/lib-2011-10/0135.html
[3] https://github.com/bagder/curl/commit/62bcf005f4678a93158358265ba905bace33b834


- wdoekes


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2403/#review8087
-----------------------------------------------------------


On March 19, 2013, 7:10 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2403/
> -----------------------------------------------------------
> 
> (Updated March 19, 2013, 7:10 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Failed CURL() requests would be quite silent.
> 
> Now you get nice warnings like this:
> 
>   WARNING[6701]: func_curl.c:654 acf_curl_helper: server certificate verification failed. \
>     CAfile: /etc/ssl/certs/ca-certificates.crt CRLfile: none ('https://ORIGINAL_URL')
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/funcs/func_curl.c 383308 
> 
> Diff: https://reviewboard.asterisk.org/r/2403/diff
> 
> 
> Testing
> -------
> 
> It works.
> 
> I also tested that setting CURLOPT_ERRORBUFFER to NULL does what you'd expect. (And the cast is to silent the fancy gcc type checks.)
> 
> 
> Thanks,
> 
> wdoekes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130319/12977529/attachment.htm>