On 1/25/16, 6:26 PM, "c-ares on behalf of bdoetsch" <c-ares-bounces_at_cool.haxx.se on behalf of bdoetsch_at_ameritech.net> wrote:
>On 1/25/16, 6:24 AM, "David Drysdale" <drysdale_at_google.com> wrote:
>
>>On Sat, Jan 23, 2016 at 9:42 PM, bdoetsch <bdoetsch_at_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
>
@Fedor -- I think you and I are trying to get the same result, a TXT parsing routine that clearly delineates between TXT chunk components. Hopefully this submission can help us both.
Daniel, David, et al.,
I've taken a second crack at a patch for this issue, taking into account the excellent advice from David to include any new structs explicitly in the headers, and give new structs their own parse routines, as opposed to casting between common initial sequences, which is what I had done the first time in an attempt to be minimal. Any feedback on this new code is welcome, I'd love more eyes on it to make sure I haven't made mistakes, and I'm happy to make modifications if need be. Because I've included associated tests, this patch is against David's git repo (https://github.com/daviddrysdale/c-ares.git), commit "2dd71de," because that is the fork that has the unit tests, but I believe is current with the main project.
I use a new "ares_txt_cmps_reply" struct which deprecates the "ares_txt_reply" struct. It is parsed by its own function "ares_parse_txt_cmps_reply()" which is a modified implementation of "ares_parse_txt_reply()". The patch adds new file "ares_parse_txt_cmps_reply.c" modifies existing files "ares.h," "ares_data.[c|h]" is documented in new man page "ares_parse_txt_cmps_reply.3," and is tested in new test file "test/ares-test-parse-txt-cmps.cc". Looking back over the mailing list, I saw that Jakub Hrozek requested adding TTL into the new struct if ever it was implemented, so I've done that as well. Here's how it looks presently:
struct ares_txt_cmps_reply {
struct ares_txt_cmps_reply *next;
unsigned char **txt_cmps; /* array of null terminated strings */
unsigned int *txt_cmps_lengths; /* length of each string in the array, don't rely on '\0' */
unsigned int txt_cmps_count; /* number of strings/lengths in the array(s) */
unsigned short dnstype;
unsigned short dnsclass;
unsigned int ttl;
};
To see the full submission, clone David's repo at the github url above, checkout commit 2dd71de, perform a git-apply on that revision using the attached patch. Let me know how you feel about the direction. I'm not married to it, so feel free to speak your mind...
My continued thanks,
Brady