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, 1 Feb 2016 16:31:07 +0000

On Thu, Jan 28, 2016 at 11:42 PM, bdoetsch <bdoetsch_at_ameritech.net> wrote:

>
> @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.
>

Hi Brady,

By the way, did you take a look at Fedor's suggested patches, particularly
the one from 2015-03-21? Would that patch cover the functionality you need?

> 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.

(BTW, I believe casting between common initial sequences is legal if
they're all in the same union, so I think that Fedor's patch is OK in that
respect.)

> 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.

Yeah, I don't actually agree with Jakub on that, as it's inconsistent with
all of the other parsed RR structures -- I think it would be better to come
up with another way of getting the TTL information out that applies to all
RR types, not just TXT records.

> 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;
>

I've not looked at the patch yet, but I'd wonder whether these two fields
are necessary -- can they ever be anything other than IN(1) / TXT(16) ?

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