On 1/25/16, 6:24 AM, "David Drysdale" <drysd...@google.com> wrote:
>On Sat, Jan 23, 2016 at 9:42 PM, bdoetsch <bdoet...@ameritech.net> wrote: >> Forgive me if I don’t follow protocol correctly, I’m new to this group. >> >> I’ve made a small patch to allow multi-part TXT responses to be >> reconstructed as either the full un-chunked message, or its component parts. >> It does not break the API or the ABI, and it aims to be as minimal as >> possible; the “ares_txt_reply” struct remains unmodified. I also tested the >> patched code against David Drysdale’s super cool test suite at >> https://github.com/daviddrysdale/c-ares/commits/test with full success. I >> also added a test to ensure that the c-ares patch is working as expected >> (the patch to the testing framework is attached as well). I Hope you find >> this worthy and helpful. Please review as per your usual standards. >> >> Thanks all for such a great piece of software, and the effort it must have >> taken. c-ares is the best. >> >> Brady > >Hi Brady, > >Thanks for the patch -- and thanks in particular for adding test support >for the new code. > >I've added some minor comments below, but my main worry is that >there's a bunch of casting to make this look like it's not an API change, >when really it is. The ares_parse_txt_reply() function is now returning >a linked list of ares_txt_reply_component structures rather than >ares_txt_reply structures, so its prototype should really reflect that. > >To put it another way, adding comments/documentation to tell the >user that it's always safe (after 1.11.0) to down-cast an ares_txt_reply >to an ares_txt_reply_component seems like the wrong approach. > >So I'd probably lean towards a new entrypoint for this, which can >probably share an implementation with the existing code (although >there might be some rules lawyering about compatibility of common >initial sequences to still worry about, cf. C99 6.5.2.3 p5-8). > >Regards, >David > >-------------- > >ares_parse_txt_reply.3: > Missing man page update. > >ares.h: > Could do with a comment for the ares_txt_reply_component structure > and/or the is_continuation field to indicate why they exist. > >ares_parse_txt_reply.c: > You're implicitly relying on the new ares_txt_reply_component structure > being smaller than anything else in the ares_data.data union. I think it > would make more sense to put the new struct into the union (with full > support in ares_data.c) and always use that internally. That should also > make the added memset() unnecessary. > Finally, I'd avoid the word "new" in the comment -- it won't be long before > this isn't new any more! > >test/ares-test-parse-txt.cc: > EXPECT_EQ(false, ..) => EXPECT_FALSE(..) > EXPECT_EQ(true, ..) => EXPECT_TRUE(..) Great comments, thanks Dave. I appreciate you taking the time to look at this. Perhaps I’ll go back to the drawing board and come up with something a little bit more intentional with its own entry point and documentation that resembles more canonical parsing strategies. Thanks again, Brady
