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, 01 Feb 2016 10:37:45 -0600

Thanks Dave,

Yes, the patch from 2015-03-21 will work for me as well. Let's go for it!

B

From: c-ares <c-ares-bounces_at_cool.haxx.se> on behalf of David Drysdale <drysdale_at_google.com>
Reply-To: c-ares hacking <c-ares_at_cool.haxx.se>
Date: Monday, February 1, 2016 at 10:31 AM
To: c-ares hacking <c-ares_at_cool.haxx.se>
Subject: Re: [PATCH] extension to ares_txt_reply parsing

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