Subject: Re: [PATCH] ares_parse_txt_reply: add `record_start` field

Re: [PATCH] ares_parse_txt_reply: add `record_start` field

From: Fedor Indutny <fedor_at_indutny.com>
Date: Wed, 3 Feb 2016 15:03:20 -0500

My bad, didn't run the code indeed. All fixed, code tested in:

https://github.com/nodejs/node/pull/5062
https://ci.nodejs.org/job/node-test-commit/2081/

Appears to be working just fine with node.js .

Updated patch in attachment.

Sorry about typos!

Thank you,
Fedor.

On Wed, Feb 3, 2016 at 10:48 AM, David Drysdale <drysdale_at_google.com> wrote:

> Still seems the name of the new function is inconsistent:
> - the code and manpage have: ares_parse_txt_reply_ext
> - the master ares.h header file has: ares_parse_txt_ext
> - the commit message has: ares_parse_txt_reply_ex
>
> Have you run this code?
>
> D.
>
>
> On Mon, Feb 1, 2016 at 11:10 PM, Fedor Indutny <fedor_at_indutny.com> wrote:
>
>> Thank you, Daniel.
>>
>> Hopefully, I have fixed all mentioned nits. I tried to do my best on the
>> documentation, but please forgive me some grammar issues since I am not a
>> native english speaker.
>>
>> If you have any other suggestions with regards to either code, docs, or
>> both. I will be more than happy to put them into the patch.
>>
>> Thanks again,
>> Fedor.
>>
>> On Mon, Feb 1, 2016 at 5:29 PM, Daniel Stenberg <daniel_at_haxx.se> wrote:
>>
>>> On Mon, 1 Feb 2016, Fedor Indutny wrote:
>>>
>>> I have made fixes to the patch from 2015-03-21, according to your
>>>> comments.
>>>>
>>>> Please take a look.
>>>>
>>>
>>> Two comments from me:
>>>
>>> 1. Please don't refer to source code file names in ares.h. That's a
>>> public header file and users of c-ares will read that and have no idea
>>> about the specific source file or what we're talking about. You could of
>>> course still explain that the new struct is a superset of the old one.
>>>
>>> 2. Regarding the documentation/functionality. The new man page paragraph
>>> says:
>>>
>>> "Continuations are linked using next pointer field in the structure"
>>>
>>> (I think it should say "*the* next pointer...")
>>>
>>> ... but it doesn't really explain how it works with any details. Should
>>> the user then consider the 'txt' areas to be joined? I don't think we can
>>> presume users will understand what a continuation is if we don't explain it!
>>>
>>> --
>>>
>>> / daniel.haxx.se
>>>
>>
>>
>

Received on 2016-02-03