On Sat, 22 Jan 2011, Vsevolod Novikov wrote:

Thanks a lot for your contribution and willingness to improve libcurl!

> Patch for 7.21.3 is applied. All available tests for asynch resolvers are
> passed on my host (Linux Ubuntu), threaded as well as c-ares. Now the only
> few files depend on c-ares, and all of them are really depend on it, like
> version.c f.e. ... except easy.c, which contains never-evaluated #ifdef.
> Anyway, resolver customizing can now be isolated in curl_config.h, setup.h,
> and host....c files.

Let me just ask so that I get this correct: you're doing this primarily to be
able to replace c-ares as the async resolver library used?

If this is the purpose, I think we would benefit from separating the terms
better internally. ares would be one backend that provides async name
resolves. Right now this patch introduces ares naming in two layers and it is
a bit confusing methinks.

I would also like to get the set of async functions required to get some small
documentation so that it gets clearer to which functions another async
resolver backend would need to provide. I would prefer if we could keep all
generic asynch code into "hostasyn.c" and have the c-ares specific code in
hostares.c and the threaded async resolver in hostthre.c. (Perhaps even
consider a rename of those to "asyncares.c" and "asyncthreded.c" or similar.)

Ultimately, a new asynch resolver backend would then just replace hostares.c /
asyncares.c with its own file with the proper set of functions, and we can
more easily provide alternative backends too.

> Probably more intensive memory-loss and early-stage-request-cancelation
> tests are required ...

It would be really cool if you could get this to use the current git version
as we've modified some of the ares stuff so your patch doesn't apply
anymore, so I haven't yet fully checked how the code looks with it applied.
Nor have I tried to run anything with it.

Unfortunately I think the existing test suite isn't covering name resolves
very good. It actually is rather crappy at that. Did you also verify that the
regular and the threader resolvers still work fine?

A first basic excercise to start with: where did the ares_dup() function go?
How can your patch just remove the call and still provide the same
functionality? I would also assume that other async resolvers would need/like
to provide the same functionality.

Some other thoughts I got when I read the patch:

You moved the async handle ("areschannel") from the SessionHandle struct to be
private for the connection struct. Was that deliberate and if so why?

You removed one of the calls to ares_cancel() but I don't see how you make any
other precautions that the resolves are all completed properly at that point.

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