Subject: Re: [PATCH] extension to ares_txt_reply parsing

Re: [PATCH] extension to ares_txt_reply parsing

From: David Drysdale <drysdale_at_google.com>
Date: Mon, 25 Jan 2016 12:24:11 +0000

On Sat, Jan 23, 2016 at 9:42 PM, bradytrix <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(..)
Received on 2016-01-25