Subject: Re: [PATCH] extension to ares_txt_reply parsing

Re: [PATCH] extension to ares_txt_reply parsing

From: bdoetsch <bdoetsch_at_ameritech.net>
Date: Mon, 25 Jan 2016 18:26:50 -0600

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
Received on 2016-01-26