Re: [PATCH] ares_parse_txt_reply: add `record_start` field
Kindly reminding you about this.
On Wed, May 27, 2015 at 3:24 PM, Fedor Indutny wrote:
> Ping? ;)
>
> On Mon, Apr 6, 2015 at 11:41 PM, Fedor Indutny wrote:
>
>> Ping ;)
>>
>> On Sat, Mar 21, 2015 at 2:38 AM, Fedor Indutny wrote:
>>
>>> Hello guys!
>>>
>>> Long time again :) Sorry, I was busy with various other stuff.
>>>
>>> Here is a patch with (hopefully) all suggested changes.
>>>
>>> Please take a look!
>>>
>>> Thank you,
>>> Fedor.
>>>
>>> On Thu, Dec 11, 2014 at 5:05 AM, Jakub Hrozek
>>> wrote:
>>>
On Thu, Dec 11, 2014 at 09:54:47AM +0100, Daniel Stenberg wrote:
> On Thu, 11 Dec 2014, Jakub Hrozek wrote:
>
> >>You want me to add a couple of `void*`?
> >
> >I was thinking:
> > uint8_t reserved[16];
>
> Another way, which is one I've used elsewhere, is this:
>
> struct larger_in_future {
> int age; /* generation of struct */
> void *member; /* always present */
> }
>
> And c-ares would always set age to 0 in the first generation. A future
> extension of the struct would bump the age to 1 and extend the struct:
>
> struct larger_in_future {
> int age; /* generation of struct */
> void *member; /* always present */
>
> /* only present if 'age' is >= 1 */
> void *later;
> }
>
> It means a program has to check age before accessing struct fields
below
> 'member' which is a bit of an annoyance, but possibly doable as the
bumping
> shouldn't happen terrible often?
I don't have a problem with that personally. It's a neat trick I haven't
used before.
>>>
>>>
>>
>
Re: [PATCH] extension to ares_txt_reply parsing
On 1/25/16, 6:24 AM, "David Drysdale" wrote: >On Sat, Jan 23, 2016 at 9:42 PM, bdoetsch 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
