On 06/03/2013 02:49 PM, Kim Vandry wrote:
> On 2013-06-03 12:14, Ben Greear wrote:
>> If you want to make a try at it, you are welcome to take my patches and
>> work on them however you wish. The curl patches depend on the c-ares patch
>> to fix some crashes in it. I posted the c-ares patch a while back, but
>> got no feedback, and I don't think it was ever pushed upstream.
>
> Hi Ben,
>
> I cherry-picked only the changes related to CURLOPT_DNS_* because I have a concern about the other change (for CURLOPT_LOCALADDR) and it is not strictly related
> anyway.
>
> Following that, I:
>
> - modified your new options so that they never pass NULL pointers to c-ares, thus avoiding the crashes you mentioned. Your c-ares improvements should still be
> pursued separately, but I wanted to do this so that new cURL could still work with an older c-ares.
>
> - added documentation for the three new options.
>
> - renamed the curl command line options from --dns_interface etc... to --dns-interface etc... because the convention for long options used for other options in
> curl as well as in other software in general is hyphens, not underscores.
>
> - made some updates to the testsuite so that it would pass.
>
> - made a couple of other very minor changes (e.g. moved lines from one commit to another).
>
> I did not add any tests, but I'm not really sure what to do about that. It seems to me that in order to test this feature, a (rudimentary) DNS server would have
> to be embedded in the testsuite!
>
> There is one thing that I find to be not quite satisfactory. The interfaces for CURLOPT_DNS_LOCAL_IP4 and CURLOPT_DNS_LOCAL_IP6 are jarringly incongruous: one
> expects a direct value in host byte order, and the other expects a pointer in network byte order. But these interfaces are inherited directly from c-ares'
> interfaces so I suppose it should be left as is?

Thanks for taking time to do this.

I think I get the blame for the bad c-ares API. It worked well for my application at
the time, but perhaps not so nice for other applications. Either way, I think we
are stuck with it now.

I think you need to change the src/tool_help part of the patch below to use dashes instead
of underlines?

https://github.com/vandry/curl/commit/83fc4719f81953fc5a7aa0f9bf8c969c986b3daa

Aside from that, the patches *look* sane, but I have not tried using your
tree.

I think auto-testing this against something local would be a pain at best,
but perhaps a script could be written that hit a well known URL specifying
various bindings to make sure it worked (or not).

I can say we've tested my tree fairly well, but of course bugs could remain,
and our test coverage is based on the features we need and not necessarily
all possible combinations of these binding options.

Thanks,
Ben

-- 
Ben Greear <greearb_at_candelatech.com>
Candela Technologies Inc  http://www.candelatech.com
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html