On Tue, 2009-07-21 at 19:09 +0200, Jakub Hrozek wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hello, > The three attached patches integrate the c-ares async resolver library > into SSSD. It is a follow-up to the code posted on this list earlier . > > [PATCH 1/3] Async DNS integration > This patch is the actual integration work with interface integrated with > tevent and talloc which follows the _send/_recv notation. It also > contains code to support build (like configure checks).
I am a bit concerned about memory hierarchies here. In fd_event_add() you add a new event context allocating it on the tevent_context memory context: + fde = tevent_add_fd(ctx->ev_ctx, ctx, s, TEVENT_FD_READ, fd_input_available, watch); This is bad because if I talloc_free() the resolv context later on on, all the events will not be freed but will stay around and watch->ctx will point to freed memory. Allocating the watch itself on the tevent_context is equally bad. Another potential problem I see is that, if I read the code correctly, resolv_gethostbyname_done() maybe called even if the original request was freed, this may cause the code to segfault when it calls tevent_req_XXX(req, ...) functions and tries to access freed memory. > [PATCH 2/3] Add ares helpers into sssd > The second patch contains ares_parse_srv_reply() and > ares_parse_txt_reply() routines that I sent to ares upstream. Also > contains configure-time detection of these in the system ares library > and builds them only if necessary. I am not an expert in c-ares, but it looks like the code is sane, one minor thing though. Unless upstream demands copyright assignment to MIT, I think we can maintain the copyright, and even if assignment is required we can do it only with the patches submitted to ares, no need to do so with code into the sssd repo itself. In any case the (C) is certainly not 1998. That is, unless you traveled back in time to write it :-) > [PATCH 3/3] Add async resolver tests > A basic unit test. One of the tests resolves a name on the Internet, so > it must be explicitly enabled. This look fine. Maybe when you take in account my comment to 1/3 you may want to add a test that does something like this: make a _send call then set a timer of 1 second and free the resolv_ctx, add another time of 1 second and then terminate. Perform this test with a firewall or a network cable disconnected so that you run into delays. Do the same but instead of freeing the resolv_ctx, simply free the tevent_req pointer returned by the _send() function. If the code survives without segfaults then it is probably correct. Ping me on IRC if you need some clarification on how to attain that. Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ Freeipa-devel mailing list Freeipafirstname.lastname@example.org https://www.redhat.com/mailman/listinfo/freeipa-devel