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

[Fedor Indutny]

Thank you so much, David!

On Thu, Feb 4, 2016 at 5:44 AM, David Drysdale  wrote:

> OK, I've added that change to the repo (and put in a quick test case for
> it).
>
> The Travis builds are running, but be warned: the coverage-generation
> build may well fail because coveralls.io is flaky at the moment.
>
> D.
>
> On Wed, Feb 3, 2016 at 8:03 PM, Fedor Indutny  wrote:
>
>> 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 
>> 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 
>>> 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  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
>


>>>
>>
>

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

[David Drysdale]

OK, I've added that change to the repo (and put in a quick test case for
it).

The Travis builds are running, but be warned: the coverage-generation build
may well fail because coveralls.io is flaky at the moment.

D.

On Wed, Feb 3, 2016 at 8:03 PM, Fedor Indutny  wrote:

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

>>>
>>>
>>
>

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

[Fedor Indutny]

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  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  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  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
>>>
>>
>>
>
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iQIcBAABAgAGBQJWslysAAoJENcGPM4Zt+iQzHoP/ipyehpP+9HTaiVMtABh6xCt
6WLartKMAdmv3/glXZrM0JWZ8VpjcZ9Dj1BnESYANbHdW1L6eGQDF+mlqMXQkePf
dDrivST6xw7Q0al2T+yqixbeCLgVzigIgKCeaJiNJkSrALWgc7w1Clz+uduiBOlo
ABsCOH3bRjoB+94LgksyDD5wt3jNMeK0iONNxj5NPSJhgN8CGPPbe8woPGqBFl8a
9E8RPY/nh8tc1DgHP9HjgV2fiY97pNkBKv9ODonfDtWNOQ3d93vO+pBNIurjjtZF
mIUe3sxb+ya2tBk7YPZTQcepWLZU3XfRaMKvbgLqcEakZPw7no/WnkM7qhNEvBBx
i2cp0Cjd8P2PEznRD3PpFLqDabhUkyGTCxTtG3pAHNrVLqqE6E8URYWOpWxtvhJ2
BpT84U/7h+Ap0U01KTJAwO3rB7ExgP7a9PATs02tX0qkKUJ6tDf/5KtIvT0zskWr
5ZaGzKlRzEMrC92t2m7kd2FrY7biDaGWDpygBPoXCmfPNv1h3r+r/gJ1dzQVmEUm
KFoE+j8w9bQfnVHAz0PWHfruwUITJfdwx8MFefmiIALG8c7wLGfKnAbAcSRTLei/
HJCC4ivR7ur+HsbJO29/SfX88NF6aaJWg3aqCKXcMg5x50rhpRnalDM17z3hmyez
rW3IVnH7Degl8imKnJAX
=fJCa
-END PGP SIGNATURE-


0001-txt-introduce-ares_parse_txt_reply_ext.patch
Description: Binary data

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

[Fedor Indutny]

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  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
>
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iQIcBAABAgAGBQJWr+WSAAoJENcGPM4Zt+iQvngP/idVfiEeFDHGUrpYlwn9YzI2
7mhTMUYIZTkhwmtMsQEJyymCiTMrSKXi/Qn9RV694dt3iM0xYBLOZUGfHACA3+4r
oQ4h7yldd/Bi2XlO0azD/mtX8mNBom8htqlLrju+Oo35Ch037XbI3DHQA7+1MQtW
h7dmx7b4qzSJPXu7iGmY1aWxSCKPD7HoPJs/4jUM/FTcm6ND2BRiUCO8b1O6znO7
OlicynED80TfCMRs0CKTup/mpkbIf8gd5YpakZtHkAVHqf7jNwldHd4I9wN5nlh5
ZPSNwJx9t9hDbTro1oAo9c11a/DOz6UWs5HdzgIWCq8a7CGBESDeWHAzU0N0VsKY
VCnqARsrBGULhhnV7obmcMnD8/tRj8ux4Xxgww/kbW3cTxCdRHBpKWgdu6KJqRj8
rxtQwiUDmxZ3djpId3vLWFPa/HDxXUH1KcM2CJ0UN46mffzmVSU+6Q5xoDSAYlvr
PvHCYG/jPCVFZ6qXED3/KR+HyXV9pRdeCVMX+lUVDNRWzKVEmt4wJ0w2h8ja8uii
ekNnTAv+5H4rW4DGKXC+sX1cSAgqg/DB/BUyNrdh4amJqpMZqhnPxNLwlwa0NPqe
QeU6pL463sg0kzPwJU3RDYMEZbrjWCphkzwNXmtBAhBg1WY6BUJXPb4h0zpNsLBS
JD3NVFhSm63KsRFBrKd9
=Hneg
-END PGP SIGNATURE-


0001-txt-introduce-ares_parse_txt_reply_ex.patch
Description: Binary data

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

[Daniel Stenberg]

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

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

[Fedor Indutny]

Hello David,

I have made fixes to the patch from 2015-03-21, according to your comments.

Please take a look.

Thank you!

On Mon, Feb 1, 2016 at 11:12 AM, David Drysdale  wrote:

> The first version of the patch still has the problem that it changes the
> size of
> an existing structure and thus alters the ABI.  Personally, I prefer the
> patch
> you sent 2015-03-21, just without the age+reserved fields (and with the
> other
> tweaks I suggested 2016-01-28).
>
> D.
>
> On Thu, Jan 28, 2016 at 11:24 PM, Fedor Indutny  wrote:
>
>> No worries at all!
>>
>> Does the first version of patch look better now?
>>
>> Thank you,
>> Fedor.
>>
>> On Thu, Jan 28, 2016 at 5:05 PM, Daniel Stenberg  wrote:
>>
>>> On Thu, 28 Jan 2016, Fedor Indutny wrote:
>>>
>>> Just FYI, this `age` thing was actually suggested by you earlier in this
 thread, but I will be more than happy to remove it.

>>>
>>> I know, and I'm sorry I lead you down that path! :-(
>>>
>>> --
>>>
>>>  / daniel.haxx.se
>>>
>>
>>
>
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iQIcBAABAgAGBQJWr8EeAAoJENcGPM4Zt+iQb4MP/R/pRn3cSTnckfuUGDPhSXiW
36B6mUOnz7WaKLTucEYsvU9CtrLsBnAMu61QyfzDIvYkswAKaFzBEcZlqf0Vq9ir
Niw07sRUelJRcnlU2yAQ75INSgCXVoVlhvGGF9UT0cKZf4UeLkXFS0oDmvtwDDDS
aEXKkLOAveknAXn0zxUy+Xv4iJgtkU+I4hR2CJu2iriNRFEZNMBh55R9gSRD8iqM
cMfR5UH0GkySiHVlTMX66CX18PjysObDVYaNYr3XnSe4UByHj18w3OCQtnahOCDd
RMx95YF/sQu9l6GQ6HuHTyMGh0FbYYBApy/s7AHAkbK6DqWPwLvSfm2TF75p7rE1
G8fH79/ye57W59bUmB3HWY1mnsIj/9/Z5/m7vo83CuSnWB34m2okIVAIRYRS4dTz
TLvMS4ELTJiJyTGJJX/xLYD1KQzEPHO1XYmiCDeNSzWM5yNMTO/u71uQsCsW37Ie
o9rT35lXncS4ynmCTdkZwACciVc/KlgaAQBRZrnBeBnORwauo14UWr1rOxf9U+ch
6Q6ZoBlEx3Etjug++0gVc9gkUiJqon4Eb7h+3CK5hGSmkOY+m4ez+Yy2VLayjnNH
ZhKsgwKQz3CSZCLKQrDBOn3jWoroJaJKGpift/Z9Hal58jaZdEQZ+vtmR99cBVtG
WqFeWnriXTE9wbo4B35r
=HqUy
-END PGP SIGNATURE-


0001-txt-introduce-ares_parse_txt_reply_ex.patch
Description: Binary data

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

[Fedor Indutny]

No worries at all!

Does the first version of patch look better now?

Thank you,
Fedor.

On Thu, Jan 28, 2016 at 5:05 PM, Daniel Stenberg  wrote:

> On Thu, 28 Jan 2016, Fedor Indutny wrote:
>
> Just FYI, this `age` thing was actually suggested by you earlier in this
>> thread, but I will be more than happy to remove it.
>>
>
> I know, and I'm sorry I lead you down that path! :-(
>
> --
>
>  / daniel.haxx.se
>

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

[Daniel Stenberg]

On Thu, 28 Jan 2016, Fedor Indutny wrote:

Just FYI, this `age` thing was actually suggested by you earlier in this 
thread, but I will be more than happy to remove it.


I know, and I'm sorry I lead you down that path! :-(

--

 / daniel.haxx.se

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

[Fedor Indutny]

In fact, this is what we have happily used in node.js for [quite a long
time][0]

[0]:
https://github.com/nodejs/node/commit/a60a9b0dbd064cd70de9400ad47421c19d29b021

On Thu, Jan 28, 2016 at 4:35 PM, Fedor Indutny  wrote:

> Hello Daniel, David,
>
> Just FYI, this `age` thing was actually suggested by you earlier in this
> thread, but I will be more than happy to remove it. May I ask you if the
> very first patch in this thread makes more sense now?
>
> Attached it just in case. If it looks better - I will amend man page with
> required info. Please let me know!
>
> Thank you,
> Fedor.
>
> On Thu, Jan 28, 2016 at 10:12 AM, Daniel Stenberg  wrote:
>
>> On Thu, 28 Jan 2016, David Drysdale wrote:
>>
>> Personally, I'd lean towards not including the age / reserved extension
>>> mechanism.  None of the other structures that describe particular resource
>>> records have it, and the first/rest marker is just an accidental omission
>>> from the TXT structure, so just having a new, complete, structure seems
>>> sensible.
>>>
>>
>> However, I'm happy to defer to Daniel's opinion, so I'd suggest leaving
>>> as-is for now.
>>>
>>
>> I think I'm inclined to agree with you David. No other funtion has such a
>> mechanism so it'll just stand out as a weird anomaly. Even though I may
>> have introduced that thought previously in this thread, I don't think I'm
>> liking it very much here. Let's make an effort to make clean and
>> understandable function APIs and we deprecate (primarily by documenting
>> that fact) those that we replace with better ones.
>>
>> Then at some point in time we can just cut out the old ones and release
>> c-ares version 2 without the old cruft.
>>
>>   - ares_data.c:163: I like to add a /* FALLTHROUGH */ comment for
>>> deliberate
>>> case fall through (I have a vague memory that it shuts up some lint
>>> warning too).
>>>
>>
>> Yes, for example: without that comment the Coverity scanner will warn for
>> it.
>>
>> --
>>
>>  / daniel.haxx.se
>>
>
>

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

[Fedor Indutny]

Hello Daniel, David,

Just FYI, this `age` thing was actually suggested by you earlier in this
thread, but I will be more than happy to remove it. May I ask you if the
very first patch in this thread makes more sense now?

Attached it just in case. If it looks better - I will amend man page with
required info. Please let me know!

Thank you,
Fedor.

On Thu, Jan 28, 2016 at 10:12 AM, Daniel Stenberg  wrote:

> On Thu, 28 Jan 2016, David Drysdale wrote:
>
> Personally, I'd lean towards not including the age / reserved extension
>> mechanism.  None of the other structures that describe particular resource
>> records have it, and the first/rest marker is just an accidental omission
>> from the TXT structure, so just having a new, complete, structure seems
>> sensible.
>>
>
> However, I'm happy to defer to Daniel's opinion, so I'd suggest leaving
>> as-is for now.
>>
>
> I think I'm inclined to agree with you David. No other funtion has such a
> mechanism so it'll just stand out as a weird anomaly. Even though I may
> have introduced that thought previously in this thread, I don't think I'm
> liking it very much here. Let's make an effort to make clean and
> understandable function APIs and we deprecate (primarily by documenting
> that fact) those that we replace with better ones.
>
> Then at some point in time we can just cut out the old ones and release
> c-ares version 2 without the old cruft.
>
>   - ares_data.c:163: I like to add a /* FALLTHROUGH */ comment for
>> deliberate
>> case fall through (I have a vague memory that it shuts up some lint
>> warning too).
>>
>
> Yes, for example: without that comment the Coverity scanner will warn for
> it.
>
> --
>
>  / daniel.haxx.se
>
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iQIcBAABAgAGBQJTNIorAAoJEPsOEJWxeXmZTDAP/0BVAUgror62mPtpTb/VlO+o
Y7HFycmhNJxkDMGtuwI1/KTzL4+juTq/7kLteBSyhlbHkxCO2ygYJYV9acNtuj3C
Qb7KitslvAHWnzWhtWlfBALI0QOHxz5GIoFWQZcqnvRzHtL6AJLxKC1duEpFsEKW
SwNuhCwO6oz3BaKw11L95GVFNacYwH3sdIs3ykUOgSv8itms36KxZI3mdqQdxxv7
o4NEFqcUsMxlPM7IDSHekXJuf6pN7lbxuhU2qllmUHiwz1qNGhGzD5JzNEqjlJNq
ggyzD+j8EIXwmZ3R+JaKbdutzIzcioq5MwdxTDqx7TOn7UayRjksXFKwY3vTe0zE
1OtnMdRdS75AVULvaXp/D+ulDrT2Yxp84ScI/EVWaheI8nEXEdN/5DeKkdEGQhtS
B+fS0+C6Qc/Z8MiBAVlmXE0AaEk5wZd1eFoMY996iZ4h/aZSoXwg8Wmi8ITOpAjK
ApMZMA3evfhO+N0HpRS/++tTnE0pTUcmAYW1rCFtPSxJEnlXdOm/jIYqTIZCYDLF
d2goef8gg4MbrOsHu7DKuPjC1+3UXdP3l84qsBlc89by2fvx08cF+YtTfEkYH7Ka
1NSRKJorshTDajziM2bwiUEr3KX6HV661ujUcbIxtNKVC+4nZ3JNYAwzePMyob7a
C48VQpBCP0ITQRa+KP3x
=9BGU
-END PGP SIGNATURE-


0001-ares_parse_txt_reply-add-record_start-field.patch
Description: Binary data

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

[Daniel Stenberg]

On Thu, 28 Jan 2016, David Drysdale wrote:

Personally, I'd lean towards not including the age / reserved extension 
mechanism.  None of the other structures that describe particular resource 
records have it, and the first/rest marker is just an accidental omission 
from the TXT structure, so just having a new, complete, structure seems 
sensible.


However, I'm happy to defer to Daniel's opinion, so I'd suggest leaving 
as-is for now.


I think I'm inclined to agree with you David. No other funtion has such a 
mechanism so it'll just stand out as a weird anomaly. Even though I may have 
introduced that thought previously in this thread, I don't think I'm liking it 
very much here. Let's make an effort to make clean and understandable function 
APIs and we deprecate (primarily by documenting that fact) those that we 
replace with better ones.


Then at some point in time we can just cut out the old ones and release c-ares 
version 2 without the old cruft.



  - ares_data.c:163: I like to add a /* FALLTHROUGH */ comment for
deliberate
case fall through (I have a vague memory that it shuts up some lint
warning too).


Yes, for example: without that comment the Coverity scanner will warn for it.

--

 / daniel.haxx.se

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

[Fedor Indutny]

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] ares_parse_txt_reply: add `record_start` field

[Fedor Indutny]

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] ares_parse_txt_reply: add `record_start` field

[Fedor Indutny]

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] ares_parse_txt_reply: add `record_start` field

[Fedor Indutny]

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


0001-txt-introduce-ares_parse_txt_reply_ex.patch
Description: Binary data


0001-txt-introduce-ares_parse_txt_reply_ex.patch.sig
Description: Binary data

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

[Jakub Hrozek]

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] ares_parse_txt_reply: add `record_start` field

[Daniel Stenberg]

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?


--

 / daniel.haxx.se

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

[Jakub Hrozek]

On Thu, Dec 11, 2014 at 09:00:28AM +0700, Fedor Indutny wrote:
> You want me to add a couple of `void*`?

I was thinking:
uint8_t reserved[16];
or similar.

I pulled the number out of thin air completely.

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

[Fedor Indutny]

You want me to add a couple of `void*`?

On Thu, Dec 11, 2014 at 6:10 AM, Daniel Stenberg  wrote:

> On Wed, 10 Dec 2014, Fedor Indutny wrote:
>
>  The only thing that is missing is an extra field.
>>
>
> What about coming up with a way to make the struct possible to change in
> the future so that we won't have to create yet another function later down
> the line?
>
> --
>
>  / daniel.haxx.se
>

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

[Daniel Stenberg]

On Wed, 10 Dec 2014, Fedor Indutny wrote:


The only thing that is missing is an extra field.


What about coming up with a way to make the struct possible to change in the 
future so that we won't have to create yet another function later down the 
line?


--

 / daniel.haxx.se

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

[Fedor Indutny]

Jakub,

In the more recent patch, I'm returning the new structure as you have just
said.

The only thing that is missing is an extra field.

Cheers,
Fedor.

On Wed, Dec 10, 2014 at 11:21 PM, Jakub Hrozek  wrote:

> On Wed, Dec 10, 2014 at 03:12:16PM +0700, Fedor Indutny wrote:
> > Daniel,
> >
> > I had a strange de-ja-vu about it, and I was right. I already did one
> patch
> > like that in past.
> >
> > Anyway, I redid it - so including both old and new versions for your
> > reviewal.
> >
> > Thank you,
> > Fedor.
>
> As a strange coincidence, I'm just writing a patch for SSSD that adds
> the possibility to also read the TTL for SRV records. I think this is a
> similar case.
>
> So could we, instead of adding a feature-specific name, add a new
> function with a less specific name (ares_parse_txt_ext() perhaps) that
> would return new structure, but the structure would also have some more
> data reserved for future use?
>

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

[Jakub Hrozek]

On Wed, Dec 10, 2014 at 03:12:16PM +0700, Fedor Indutny wrote:
> Daniel,
> 
> I had a strange de-ja-vu about it, and I was right. I already did one patch
> like that in past.
> 
> Anyway, I redid it - so including both old and new versions for your
> reviewal.
> 
> Thank you,
> Fedor.

As a strange coincidence, I'm just writing a patch for SSSD that adds
the possibility to also read the TTL for SRV records. I think this is a
similar case.

So could we, instead of adding a feature-specific name, add a new
function with a less specific name (ares_parse_txt_ext() perhaps) that
would return new structure, but the structure would also have some more
data reserved for future use?

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

[Fedor Indutny]

Daniel,

I had a strange de-ja-vu about it, and I was right. I already did one patch
like that in past.

Anyway, I redid it - so including both old and new versions for your
reviewal.

Thank you,
Fedor.

On Wed, Dec 10, 2014 at 12:28 PM, Fedor Indutny  wrote:

> Ok, sounds like a plan.
>
> On Wed, Dec 10, 2014 at 6:28 AM, Daniel Stenberg  wrote:
>
>> On Fri, 5 Dec 2014, Fedor Indutny wrote:
>>
>>  We kind of did, but never decided on a way of doing it. Do you have any
>>> suggestions?
>>>
>>
>> We need a new function that returns another struct so that the existing
>> applications remain functional and new ones can use the new shiny.
>>
>> --
>>
>>  / daniel.haxx.se
>>
>
>


0001-ares_parse_txt_reply-ares_parse_chunked_txt_reply.patch
Description: Binary data


0001-txt-introduce-ares_parse_txt_reply_ex.patch
Description: Binary data

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

[Fedor Indutny]

Ok, sounds like a plan.

On Wed, Dec 10, 2014 at 6:28 AM, Daniel Stenberg  wrote:

> On Fri, 5 Dec 2014, Fedor Indutny wrote:
>
>  We kind of did, but never decided on a way of doing it. Do you have any
>> suggestions?
>>
>
> We need a new function that returns another struct so that the existing
> applications remain functional and new ones can use the new shiny.
>
> --
>
>  / daniel.haxx.se
>

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

[Daniel Stenberg]

On Fri, 5 Dec 2014, Fedor Indutny wrote:

We kind of did, but never decided on a way of doing it. Do you have any 
suggestions?


We need a new function that returns another struct so that the existing 
applications remain functional and new ones can use the new shiny.


--

 / daniel.haxx.se

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

[Fedor Indutny]

Daniel,

We kind of did, but never decided on a way of doing it. Do you have any
suggestions?

Cheers,
Fedor.

On Fri, Dec 5, 2014 at 1:54 AM, Daniel Stenberg  wrote:

> On Fri, 5 Dec 2014, Fedor Indutny wrote:
>
>  The patch was in the first email from this thread.
>>
>
> Yes, like almost 200 years ago =)
>
>  Let me re-paste it here just in case.
>>
>
> Thanks! Didn't we already agree that we cannot modify the public struct
> like that as it risks break existing users of that call?
>
> --
>
>  / daniel.haxx.se
>

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

[Daniel Stenberg]

On Fri, 5 Dec 2014, Fedor Indutny wrote:


The patch was in the first email from this thread.


Yes, like almost 200 years ago =)


Let me re-paste it here just in case.


Thanks! Didn't we already agree that we cannot modify the public struct like 
that as it risks break existing users of that call?


--

 / daniel.haxx.se

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

[Fedor Indutny]

Hello Daniel!

The patch was in the first email from this thread. Let me re-paste it here
just in case.

Regarding node.js - yeah, users asked us to fix the TXT replies and we
waited for c-ares upstream to land the patch, but since it didn't happen -
we floated the patch on our own. I don't say that c-ares is causing any
problems, it is ourselves causing problems on us :) However I'd still be
very pleased if you'll land this patch or help me get it into upstream.

Thank you,
Fedor.

On Fri, Dec 5, 2014 at 12:59 AM, Daniel Stenberg  wrote:

> On Thu, 4 Dec 2014, Fedor Indutny wrote:
>
>  I still would like to ask you to land this in the form it has right now.
>>
>
> You'd simplify our lives if you'd also attach the patch you're talking
> about, or a link to it!
>
>  This has already caused some problems with incompatibility between c-ares
>> in node.js and the distributed shared library.
>>
>
> How so? You mean they went ahead and patched their embedded library
> version and then somehow users mix the standard build and their custom
> version?
>
> --
>
>  / daniel.haxx.se
>
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iQIcBAABAgAGBQJTNIorAAoJEPsOEJWxeXmZTDAP/0BVAUgror62mPtpTb/VlO+o
Y7HFycmhNJxkDMGtuwI1/KTzL4+juTq/7kLteBSyhlbHkxCO2ygYJYV9acNtuj3C
Qb7KitslvAHWnzWhtWlfBALI0QOHxz5GIoFWQZcqnvRzHtL6AJLxKC1duEpFsEKW
SwNuhCwO6oz3BaKw11L95GVFNacYwH3sdIs3ykUOgSv8itms36KxZI3mdqQdxxv7
o4NEFqcUsMxlPM7IDSHekXJuf6pN7lbxuhU2qllmUHiwz1qNGhGzD5JzNEqjlJNq
ggyzD+j8EIXwmZ3R+JaKbdutzIzcioq5MwdxTDqx7TOn7UayRjksXFKwY3vTe0zE
1OtnMdRdS75AVULvaXp/D+ulDrT2Yxp84ScI/EVWaheI8nEXEdN/5DeKkdEGQhtS
B+fS0+C6Qc/Z8MiBAVlmXE0AaEk5wZd1eFoMY996iZ4h/aZSoXwg8Wmi8ITOpAjK
ApMZMA3evfhO+N0HpRS/++tTnE0pTUcmAYW1rCFtPSxJEnlXdOm/jIYqTIZCYDLF
d2goef8gg4MbrOsHu7DKuPjC1+3UXdP3l84qsBlc89by2fvx08cF+YtTfEkYH7Ka
1NSRKJorshTDajziM2bwiUEr3KX6HV661ujUcbIxtNKVC+4nZ3JNYAwzePMyob7a
C48VQpBCP0ITQRa+KP3x
=9BGU
-END PGP SIGNATURE-


0001-ares_parse_txt_reply-add-record_start-field.patch
Description: Binary data

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

[Daniel Stenberg]

On Thu, 4 Dec 2014, Fedor Indutny wrote:


I still would like to ask you to land this in the form it has right now.


You'd simplify our lives if you'd also attach the patch you're talking about, 
or a link to it!


This has already caused some problems with incompatibility between c-ares in 
node.js and the distributed shared library.


How so? You mean they went ahead and patched their embedded library version 
and then somehow users mix the standard build and their custom version?


--

 / daniel.haxx.se

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

[Fedor Indutny]

I still would like to ask you to land this in the form it has right now.

This has already caused some problems with incompatibility between c-ares
in node.js and the distributed shared library.

Please let me know how I could help you proceeding with this.

Thank you,
Fedor.

On Thu, Jun 12, 2014 at 3:15 AM, Fedor Indutny  wrote:

> I think it applies to each record, otherwise it would indeed just an
> additional `int*` argument.
>
> Any ideas? Perhaps we could land it in a way it is right now?
>
>
> On Wed, Jun 11, 2014 at 2:40 AM, Saúl Ibarra Corretgé 
> wrote:
>
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA1
>>
>> On 06/06/2014 06:30 AM, Fedor Indutny wrote:
>> > Thinking about it, I have only one hacky idea for implementing it
>> > without bloating the code base, or allocating excessive bytes.
>> >
>> > The TTL could be put into the `len` field of record when `txt` is
>> > `NULL`.
>> >
>> > Does it seems good to you guys?
>> >
>>
>> Sounds a bit hacky... Not sure if TTL applies to each individual
>> record or it's global, in that case it would just be an extra int* on
>> the function, right?
>>
>> >
>> > On Wed, May 28, 2014 at 6:12 PM, Fedor Indutny > > > wrote:
>> >
>> > Oh right, will do it.
>> >
>> >
>> > On Wed, May 28, 2014 at 4:59 PM, Saúl Ibarra Corretgé
>> > mailto:sag...@gmail.com>> wrote:
>> >
>> > On 05/27/2014 12:01 PM, Fedor Indutny wrote:
>> >> Sure, here are attached changes.
>> >
>> >> Please let me know if I should improve them even further.
>> >
>> >
>> > Looks good to me, FWIW.
>> >
>> > Someone asked if you could add the TTL data there, maybe taking
>> > the same approach as ares_parse_a_reply? Something like:
>> >
>> > ares_parse_chunked_txt_reply (const unsigned char *abuf, int alen,
>> > struct ares_txt_reply **txt_out, struct ares_addrttl *addrttls, int
>> > *naddrttls)
>> >
>> >
>> > Cheers,
>> >
>> >
>> >
>> >
>>
>> - --
>> Saúl Ibarra Corretgé
>> bettercallsaghul.com
>>
>> -BEGIN PGP SIGNATURE-
>> Version: GnuPG v1
>> Comment: Using GnuPG with Icedove - http://www.enigmail.net/
>>
>> iQIcBAEBAgAGBQJTmCPzAAoJEEEOVVOum8BZULAQAKzdhJQTf3g/jexKNlp7exu6
>> iGsF2nAncmGXeCsu/aKdUzoTD4ouuPiEVM1yOuNLpEFB05yMEGP+Pq1P2hL0UPKU
>> eRdetfeFH+eafhyOtPSvWr/f+AJMBHl/Q6U9Tp6eeif/hynfUBf0YLYUqKhScJZL
>> x2UnLIS2fnE/MCwWsZhGIy71TbrqLminNMopC6Osek69LsRwCi7+377GyNH2yklv
>> nBdWdUWplCfNdYnjG8T4lXLm0Mk26HEaQYQWN1ZtzLx9unTBCdWIxjnrrkdBx6PF
>> RlIrTPqXDkU/lpyc9BcW3KlfIK6Wngdu1RLlr6RIY3dCkM+Sq0UW41pf7Hm/6Pq4
>> aMUBga/RLMGdVEbu5elfO5J1U5hDiHP4TV6CoCTAuQ4hlKz07i0XTcuDfxJcexsU
>> fcxtm16dodV/0sk0m8dON+jcHZU2A/CJMaaWqbRoGCEH7IJ2Bdx8d4seH3MgHVzK
>> tAFWaWSBYfqt7XMphT0i2sn8STOHj9E/JkGykOZ8PFLGEDWwdLLd7VKqiUUv6N6z
>> 3PCjJsZ13j32CMVC9cWbVzJjmZS14YQViujBUDcLXU4d/XlbgGYtGucUJPfq9REP
>> RUudttoysRamAaIdAt2Sfc8t6Kr+xhTTa7rMOgJwwaaXGaEg09w/lALAdV6b3Q2/
>> uo3522LRpX7t4o3xaOfv
>> =3AYx
>> -END PGP SIGNATURE-
>>
>
>

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

[Fedor Indutny]

I think it applies to each record, otherwise it would indeed just an
additional `int*` argument.

Any ideas? Perhaps we could land it in a way it is right now?


On Wed, Jun 11, 2014 at 2:40 AM, Saúl Ibarra Corretgé 
wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 06/06/2014 06:30 AM, Fedor Indutny wrote:
> > Thinking about it, I have only one hacky idea for implementing it
> > without bloating the code base, or allocating excessive bytes.
> >
> > The TTL could be put into the `len` field of record when `txt` is
> > `NULL`.
> >
> > Does it seems good to you guys?
> >
>
> Sounds a bit hacky... Not sure if TTL applies to each individual
> record or it's global, in that case it would just be an extra int* on
> the function, right?
>
> >
> > On Wed, May 28, 2014 at 6:12 PM, Fedor Indutny  > > wrote:
> >
> > Oh right, will do it.
> >
> >
> > On Wed, May 28, 2014 at 4:59 PM, Saúl Ibarra Corretgé
> > mailto:sag...@gmail.com>> wrote:
> >
> > On 05/27/2014 12:01 PM, Fedor Indutny wrote:
> >> Sure, here are attached changes.
> >
> >> Please let me know if I should improve them even further.
> >
> >
> > Looks good to me, FWIW.
> >
> > Someone asked if you could add the TTL data there, maybe taking
> > the same approach as ares_parse_a_reply? Something like:
> >
> > ares_parse_chunked_txt_reply (const unsigned char *abuf, int alen,
> > struct ares_txt_reply **txt_out, struct ares_addrttl *addrttls, int
> > *naddrttls)
> >
> >
> > Cheers,
> >
> >
> >
> >
>
> - --
> Saúl Ibarra Corretgé
> bettercallsaghul.com
>
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1
> Comment: Using GnuPG with Icedove - http://www.enigmail.net/
>
> iQIcBAEBAgAGBQJTmCPzAAoJEEEOVVOum8BZULAQAKzdhJQTf3g/jexKNlp7exu6
> iGsF2nAncmGXeCsu/aKdUzoTD4ouuPiEVM1yOuNLpEFB05yMEGP+Pq1P2hL0UPKU
> eRdetfeFH+eafhyOtPSvWr/f+AJMBHl/Q6U9Tp6eeif/hynfUBf0YLYUqKhScJZL
> x2UnLIS2fnE/MCwWsZhGIy71TbrqLminNMopC6Osek69LsRwCi7+377GyNH2yklv
> nBdWdUWplCfNdYnjG8T4lXLm0Mk26HEaQYQWN1ZtzLx9unTBCdWIxjnrrkdBx6PF
> RlIrTPqXDkU/lpyc9BcW3KlfIK6Wngdu1RLlr6RIY3dCkM+Sq0UW41pf7Hm/6Pq4
> aMUBga/RLMGdVEbu5elfO5J1U5hDiHP4TV6CoCTAuQ4hlKz07i0XTcuDfxJcexsU
> fcxtm16dodV/0sk0m8dON+jcHZU2A/CJMaaWqbRoGCEH7IJ2Bdx8d4seH3MgHVzK
> tAFWaWSBYfqt7XMphT0i2sn8STOHj9E/JkGykOZ8PFLGEDWwdLLd7VKqiUUv6N6z
> 3PCjJsZ13j32CMVC9cWbVzJjmZS14YQViujBUDcLXU4d/XlbgGYtGucUJPfq9REP
> RUudttoysRamAaIdAt2Sfc8t6Kr+xhTTa7rMOgJwwaaXGaEg09w/lALAdV6b3Q2/
> uo3522LRpX7t4o3xaOfv
> =3AYx
> -END PGP SIGNATURE-
>

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

[Saúl Ibarra Corretgé]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 06/06/2014 06:30 AM, Fedor Indutny wrote:
> Thinking about it, I have only one hacky idea for implementing it 
> without bloating the code base, or allocating excessive bytes.
> 
> The TTL could be put into the `len` field of record when `txt` is
> `NULL`.
> 
> Does it seems good to you guys?
> 

Sounds a bit hacky... Not sure if TTL applies to each individual
record or it's global, in that case it would just be an extra int* on
the function, right?

> 
> On Wed, May 28, 2014 at 6:12 PM, Fedor Indutny  > wrote:
> 
> Oh right, will do it.
> 
> 
> On Wed, May 28, 2014 at 4:59 PM, Saúl Ibarra Corretgé 
> mailto:sag...@gmail.com>> wrote:
> 
> On 05/27/2014 12:01 PM, Fedor Indutny wrote:
>> Sure, here are attached changes.
> 
>> Please let me know if I should improve them even further.
> 
> 
> Looks good to me, FWIW.
> 
> Someone asked if you could add the TTL data there, maybe taking
> the same approach as ares_parse_a_reply? Something like:
> 
> ares_parse_chunked_txt_reply (const unsigned char *abuf, int alen, 
> struct ares_txt_reply **txt_out, struct ares_addrttl *addrttls, int
> *naddrttls)
> 
> 
> Cheers,
> 
> 
> 
> 

- -- 
Saúl Ibarra Corretgé
bettercallsaghul.com

-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iQIcBAEBAgAGBQJTmCPzAAoJEEEOVVOum8BZULAQAKzdhJQTf3g/jexKNlp7exu6
iGsF2nAncmGXeCsu/aKdUzoTD4ouuPiEVM1yOuNLpEFB05yMEGP+Pq1P2hL0UPKU
eRdetfeFH+eafhyOtPSvWr/f+AJMBHl/Q6U9Tp6eeif/hynfUBf0YLYUqKhScJZL
x2UnLIS2fnE/MCwWsZhGIy71TbrqLminNMopC6Osek69LsRwCi7+377GyNH2yklv
nBdWdUWplCfNdYnjG8T4lXLm0Mk26HEaQYQWN1ZtzLx9unTBCdWIxjnrrkdBx6PF
RlIrTPqXDkU/lpyc9BcW3KlfIK6Wngdu1RLlr6RIY3dCkM+Sq0UW41pf7Hm/6Pq4
aMUBga/RLMGdVEbu5elfO5J1U5hDiHP4TV6CoCTAuQ4hlKz07i0XTcuDfxJcexsU
fcxtm16dodV/0sk0m8dON+jcHZU2A/CJMaaWqbRoGCEH7IJ2Bdx8d4seH3MgHVzK
tAFWaWSBYfqt7XMphT0i2sn8STOHj9E/JkGykOZ8PFLGEDWwdLLd7VKqiUUv6N6z
3PCjJsZ13j32CMVC9cWbVzJjmZS14YQViujBUDcLXU4d/XlbgGYtGucUJPfq9REP
RUudttoysRamAaIdAt2Sfc8t6Kr+xhTTa7rMOgJwwaaXGaEg09w/lALAdV6b3Q2/
uo3522LRpX7t4o3xaOfv
=3AYx
-END PGP SIGNATURE-

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

[Fedor Indutny]

Thinking about it, I have only one hacky idea for implementing it without
bloating the code base, or allocating excessive bytes.

The TTL could be put into the `len` field of record when `txt` is `NULL`.

Does it seems good to you guys?


On Wed, May 28, 2014 at 6:12 PM, Fedor Indutny  wrote:

> Oh right, will do it.
>
>
> On Wed, May 28, 2014 at 4:59 PM, Saúl Ibarra Corretgé 
> wrote:
>
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA1
>>
>> On 05/27/2014 12:01 PM, Fedor Indutny wrote:
>> > Sure, here are attached changes.
>> >
>> > Please let me know if I should improve them even further.
>> >
>>
>> Looks good to me, FWIW.
>>
>> Someone asked if you could add the TTL data there, maybe taking the
>> same approach as ares_parse_a_reply? Something like:
>>
>> ares_parse_chunked_txt_reply (const unsigned char *abuf,
>>   int alen,
>>   struct ares_txt_reply **txt_out,
>>   struct ares_addrttl *addrttls,
>>   int *naddrttls)
>>
>>
>> Cheers,
>>
>> - --
>> Saúl Ibarra Corretgé
>> bettercallsaghul.com
>>
>> -BEGIN PGP SIGNATURE-
>> Version: GnuPG v1
>> Comment: Using GnuPG with Icedove - http://www.enigmail.net/
>>
>> iQIcBAEBAgAGBQJThd2tAAoJEEEOVVOum8BZR9MP/RMgT5oqH4ZmSaQ41Zo9Igpe
>> qwy3SJm5YY/P/fa16uN2m7kJbyU5ksThUVj9c8PDA6fYecEqZTQMHY+we3UYOv1w
>> rwp2/GqAEv3Sx6fjdQ9Rm8KVwIDQRL8wb7DlxyMYiK+Oae18tokx/gFacEUA9g0F
>> ulH/uiBXgEnmDTq38H9rMTWn14PWbeHYl0XQNHDrGBAJkKuY+xnPmy+STG0ER5E0
>> dGIVfi2TgtEUhgnj6eyOqNLIV1cZDXBerUoIiXtVDejZNRSXE/Y59SPreCNgV+ZF
>> l43LPVbenP2EAlFUnZSq+OcD+/kQLuYs74jgSK5K2FyBMozhkLn84Vf/oDn8C9po
>> nVWpt8DmiJAlJ+QUkEU1s6SA6LWSF2ogN3ekTyFgN8buicNgS5B4xPFSqsU5E1IJ
>> eigEB1Wjvsz0bJDlfxi4ztNpd/5WK7mmiexO4HBXkY+RfbWpBBKvt6DkMjJsNtQa
>> XzvHsRIXIfzEp8/yigJ0gYhNkuOpaD4I5yTkbixVvbnd01dC7zON9sBNuCsqwnPj
>> dRQE5QauM28hUpkBLHtsMKGRclzOMVHGb1GKewX8qSPAFRoOHRGdVr786w8afYMe
>> /LZCWEJScohwq00X4JvT5qL8C6oWWqYrRXL93K1DK64tS1k5wyT/aN7F45RA1jLL
>> 8n+WOaaz6krHdWV3LF+Y
>> =vsvs
>> -END PGP SIGNATURE-
>>
>
>

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

[Fedor Indutny]

Oh right, will do it.


On Wed, May 28, 2014 at 4:59 PM, Saúl Ibarra Corretgé wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 05/27/2014 12:01 PM, Fedor Indutny wrote:
> > Sure, here are attached changes.
> >
> > Please let me know if I should improve them even further.
> >
>
> Looks good to me, FWIW.
>
> Someone asked if you could add the TTL data there, maybe taking the
> same approach as ares_parse_a_reply? Something like:
>
> ares_parse_chunked_txt_reply (const unsigned char *abuf,
>   int alen,
>   struct ares_txt_reply **txt_out,
>   struct ares_addrttl *addrttls,
>   int *naddrttls)
>
>
> Cheers,
>
> - --
> Saúl Ibarra Corretgé
> bettercallsaghul.com
>
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1
> Comment: Using GnuPG with Icedove - http://www.enigmail.net/
>
> iQIcBAEBAgAGBQJThd2tAAoJEEEOVVOum8BZR9MP/RMgT5oqH4ZmSaQ41Zo9Igpe
> qwy3SJm5YY/P/fa16uN2m7kJbyU5ksThUVj9c8PDA6fYecEqZTQMHY+we3UYOv1w
> rwp2/GqAEv3Sx6fjdQ9Rm8KVwIDQRL8wb7DlxyMYiK+Oae18tokx/gFacEUA9g0F
> ulH/uiBXgEnmDTq38H9rMTWn14PWbeHYl0XQNHDrGBAJkKuY+xnPmy+STG0ER5E0
> dGIVfi2TgtEUhgnj6eyOqNLIV1cZDXBerUoIiXtVDejZNRSXE/Y59SPreCNgV+ZF
> l43LPVbenP2EAlFUnZSq+OcD+/kQLuYs74jgSK5K2FyBMozhkLn84Vf/oDn8C9po
> nVWpt8DmiJAlJ+QUkEU1s6SA6LWSF2ogN3ekTyFgN8buicNgS5B4xPFSqsU5E1IJ
> eigEB1Wjvsz0bJDlfxi4ztNpd/5WK7mmiexO4HBXkY+RfbWpBBKvt6DkMjJsNtQa
> XzvHsRIXIfzEp8/yigJ0gYhNkuOpaD4I5yTkbixVvbnd01dC7zON9sBNuCsqwnPj
> dRQE5QauM28hUpkBLHtsMKGRclzOMVHGb1GKewX8qSPAFRoOHRGdVr786w8afYMe
> /LZCWEJScohwq00X4JvT5qL8C6oWWqYrRXL93K1DK64tS1k5wyT/aN7F45RA1jLL
> 8n+WOaaz6krHdWV3LF+Y
> =vsvs
> -END PGP SIGNATURE-
>

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

[Saúl Ibarra Corretgé]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 05/27/2014 12:01 PM, Fedor Indutny wrote:
> Sure, here are attached changes.
> 
> Please let me know if I should improve them even further.
> 

Looks good to me, FWIW.

Someone asked if you could add the TTL data there, maybe taking the
same approach as ares_parse_a_reply? Something like:

ares_parse_chunked_txt_reply (const unsigned char *abuf,
  int alen,
  struct ares_txt_reply **txt_out,
  struct ares_addrttl *addrttls,
  int *naddrttls)


Cheers,

- -- 
Saúl Ibarra Corretgé
bettercallsaghul.com

-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iQIcBAEBAgAGBQJThd2tAAoJEEEOVVOum8BZR9MP/RMgT5oqH4ZmSaQ41Zo9Igpe
qwy3SJm5YY/P/fa16uN2m7kJbyU5ksThUVj9c8PDA6fYecEqZTQMHY+we3UYOv1w
rwp2/GqAEv3Sx6fjdQ9Rm8KVwIDQRL8wb7DlxyMYiK+Oae18tokx/gFacEUA9g0F
ulH/uiBXgEnmDTq38H9rMTWn14PWbeHYl0XQNHDrGBAJkKuY+xnPmy+STG0ER5E0
dGIVfi2TgtEUhgnj6eyOqNLIV1cZDXBerUoIiXtVDejZNRSXE/Y59SPreCNgV+ZF
l43LPVbenP2EAlFUnZSq+OcD+/kQLuYs74jgSK5K2FyBMozhkLn84Vf/oDn8C9po
nVWpt8DmiJAlJ+QUkEU1s6SA6LWSF2ogN3ekTyFgN8buicNgS5B4xPFSqsU5E1IJ
eigEB1Wjvsz0bJDlfxi4ztNpd/5WK7mmiexO4HBXkY+RfbWpBBKvt6DkMjJsNtQa
XzvHsRIXIfzEp8/yigJ0gYhNkuOpaD4I5yTkbixVvbnd01dC7zON9sBNuCsqwnPj
dRQE5QauM28hUpkBLHtsMKGRclzOMVHGb1GKewX8qSPAFRoOHRGdVr786w8afYMe
/LZCWEJScohwq00X4JvT5qL8C6oWWqYrRXL93K1DK64tS1k5wyT/aN7F45RA1jLL
8n+WOaaz6krHdWV3LF+Y
=vsvs
-END PGP SIGNATURE-

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

[Fedor Indutny]

Sure, here are attached changes.

Please let me know if I should improve them even further.


On Tue, May 27, 2014 at 12:39 PM, Jakub Hrozek  wrote:

> On Tue, May 27, 2014 at 12:53:41AM +0400, Fedor Indutny wrote:
> > Agreed, will look into implementing it.
>
> Would you consider also adding TTL when creating this "ares_parse_txt2"
> ?
>
> >
> >
> > On Tue, May 27, 2014 at 12:50 AM, Daniel Stenberg 
> wrote:
> >
> > > On Thu, 22 May 2014, Fedor Indutny wrote:
> > >
> > >  Unfortunately, there is no certain description of how TXT records
> should
> > >> be used after parsing. Clearly some people may want to get each chunk
> of
> > >> each record separately and use them as they want to. Concatenation
> will
> > >> definitely make c-ares unusable for them.
> > >>
> > >
> > > An option is to introduce another function that can return this info
> and
> > > yet we maintain API and ABI backwards compatibility.
> > >
> > > --
> > >
> > >  / daniel.haxx.se
> > >
>
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

- From 2dd4eec675ccc605cc88cc2099b109c2db05e128 Mon Sep 17 00:00:00 2001
From: Fedor Indutny 
Date: Fri, 28 Mar 2014 00:14:59 +0400
Subject: [PATCH] ares_parse_txt_reply: ares_parse_chunked_txt_reply

Introduce `ares_parse_chunked_txt_reply` function, that inserts empty
TXT chunks at the record boundary. With this information it is possible
to distinguish chunks from one record from chunks of another.
- ---
 ares.h |  3 +++
 ares_parse_txt_reply.c | 35 +++
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/ares.h b/ares.h
index 9b3f376..b74080a 100644
- --- a/ares.h
+++ b/ares.h
@@ -536,6 +536,9 @@ CARES_EXTERN int ares_parse_mx_reply(const unsigned char* 
abuf,
 CARES_EXTERN int ares_parse_txt_reply(const unsigned char* abuf,
   int alen,
   struct ares_txt_reply** txt_out);
+CARES_EXTERN int ares_parse_chunked_txt_reply(const unsigned char* abuf,
+  int alen,
+  struct ares_txt_reply** txt_out);
 
 CARES_EXTERN int ares_parse_naptr_reply(const unsigned char* abuf,
 int alen,
diff --git a/ares_parse_txt_reply.c b/ares_parse_txt_reply.c
index 981db4c..52662a8 100644
- --- a/ares_parse_txt_reply.c
+++ b/ares_parse_txt_reply.c
@@ -45,8 +45,8 @@
 #include "ares_private.h"
 
 int
- -ares_parse_txt_reply (const unsigned char *abuf, int alen,
- -  struct ares_txt_reply **txt_out)
+ares__parse_txt_reply (const unsigned char *abuf, int alen,
+   int chunked, struct ares_txt_reply **txt_out)
 {
   size_t substr_len;
   unsigned int qdcount, ancount, i;
@@ -54,6 +54,7 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
   const unsigned char *strptr;
   int status, rr_type, rr_class, rr_len;
   long len;
+  unsigned char record_start;
   char *hostname = NULL, *rr_name = NULL;
   struct ares_txt_reply *txt_head = NULL;
   struct ares_txt_reply *txt_last = NULL;
@@ -133,8 +134,6 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
   break;
 }
 
- -  ++strptr;
- -
   /* Allocate storage for this TXT answer appending it to the list 
*/
   txt_curr = ares_malloc_data(ARES_DATATYPE_TXT_REPLY);
   if (!txt_curr)
@@ -142,6 +141,12 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
   status = ARES_ENOMEM;
   break;
 }
+
+  /* Mark record starts with { .txt = NULL, .length = 0 } chunks */
+  record_start = chunked &&
+ strptr == aptr &&
+ txt_last != NULL &&
+ txt_last->txt != NULL;
   if (txt_last)
 {
   txt_last->next = txt_curr;
@@ -152,6 +157,12 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
 }
   txt_last = txt_curr;
 
+  if (record_start) {
+txt_curr->txt = NULL;
+txt_curr->length = 0;
+continue;
+  }
+
   txt_curr->length = substr_len;
   txt_curr->txt = malloc (substr_len + 1/* Including null byte */);
   if (txt_curr->txt == NULL)
@@ -159,6 +170,8 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
   status = ARES_ENOMEM;
   break;
 }
+
+  ++strptr;
   memcpy ((char *) txt_curr->txt, strptr, substr_len);
 
   /* Make sure we NULL-terminate */
@@ -194,3 +207,17 @@ ares_parse_txt_reply (const unsigned char *abuf, int alen,
 
   return ARES_SUCCESS;
 }
+
+int
+ares_parse_txt_reply (const unsigned char *abuf, int alen,
+  struct ares_txt_reply **txt_out)
+{
+

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

[Jakub Hrozek]

On Tue, May 27, 2014 at 12:53:41AM +0400, Fedor Indutny wrote:
> Agreed, will look into implementing it.

Would you consider also adding TTL when creating this "ares_parse_txt2"
?

> 
> 
> On Tue, May 27, 2014 at 12:50 AM, Daniel Stenberg  wrote:
> 
> > On Thu, 22 May 2014, Fedor Indutny wrote:
> >
> >  Unfortunately, there is no certain description of how TXT records should
> >> be used after parsing. Clearly some people may want to get each chunk of
> >> each record separately and use them as they want to. Concatenation will
> >> definitely make c-ares unusable for them.
> >>
> >
> > An option is to introduce another function that can return this info and
> > yet we maintain API and ABI backwards compatibility.
> >
> > --
> >
> >  / daniel.haxx.se
> >

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

[Fedor Indutny]

Agreed, will look into implementing it.


On Tue, May 27, 2014 at 12:50 AM, Daniel Stenberg  wrote:

> On Thu, 22 May 2014, Fedor Indutny wrote:
>
>  Unfortunately, there is no certain description of how TXT records should
>> be used after parsing. Clearly some people may want to get each chunk of
>> each record separately and use them as they want to. Concatenation will
>> definitely make c-ares unusable for them.
>>
>
> An option is to introduce another function that can return this info and
> yet we maintain API and ABI backwards compatibility.
>
> --
>
>  / daniel.haxx.se
>

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

[Daniel Stenberg]

On Thu, 22 May 2014, Fedor Indutny wrote:

Unfortunately, there is no certain description of how TXT records should be 
used after parsing. Clearly some people may want to get each chunk of each 
record separately and use them as they want to. Concatenation will 
definitely make c-ares unusable for them.


An option is to introduce another function that can return this info and yet 
we maintain API and ABI backwards compatibility.


--

 / daniel.haxx.se

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

[Fedor Indutny]

Hello Saul!

As an alternative, I could use `NULL` txt field to indicate a record
separation, but this
will break the existing code.

Unfortunately, there is no certain description of how TXT records should be
used after
parsing. Clearly some people may want to get each chunk of each record
separately
and use them as they want to. Concatenation will definitely make c-ares
unusable for
them.

Cheers,
Fedor.


On Thu, May 22, 2014 at 10:51 AM, Saúl Ibarra Corretgé wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 05/19/2014 09:19 PM, Fedor Indutny wrote:
> > Introduce `record_start` field, indicating start of new TXT record,
> > thus allowing to differentiate chunks in the same record, from a
> > chunks in a different record.
>
> Hey Fedor!
>
> This patch changes the ABI, any chance this could be done by
> concatenating the chunks and thus not requiring that extra field in
> the ares_txt_reply struct?
>
> - --
> Saúl Ibarra Corretgé
> bettercallsaghul.com
>
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1
> Comment: Using GnuPG with Icedove - http://www.enigmail.net/
>
> iQIcBAEBAgAGBQJTfZ5fAAoJEEEOVVOum8BZkPkQAI/0KXcb+jyv17pgWZvHzoyD
> DesPcht2ZXh3l47aIdWrbQlVlgDqtqFmYT6eiMVoG03M/0C6uySXUGzqVz5LTFf1
> EBiHGluGNbigHAfv8Rj+Cm++fDkuyGdDpwwqWhgytGGKHB/SCA8wlCEFr+eFgxEK
> z2+sy0NKFGOZdZc1sholju8gYalUvTD3buKHgl0vKGcBTCIZOn12IiaySzeILHp/
> FlBFl6v8ihqg8xEynhg4I9F/2AMWSjRXYjxyCB2lcc+9yAgy6QcSu6nimogstlM+
> grlY3aMscVfEXAy4bmAFfKgxcbma6SX7tRDVNjEM2jZQITxL2+/MBEo00ny0YdDx
> IYbUbjEBQSpNHkQtb9QQofgDfYHXCbsNjy5cNpIDHA2ENCySYzRkNAz2I/36p9g9
> RigP8MYqYrgbZqjZsP47PSFlrmdhTGzWqss8dfLjJoQeIdmUnrLaUdU7h7mIGveq
> IP3fbWhMoYJYnsvL3DP0Tvv/ChsitNuOkHIKP+xtOTZK/YMucJylxVNV99RvqvKZ
> UY4SurNd60hN5N89+m92Yrgu8wokCj3Ya3XPmNd2Ufo18NrA1t7NQSPSo/h9frek
> Sjaquzv2GmpnvLKO8dCJw0PwpIve1+T4weWk9kjstXbr8R55g5+4V3lbfI7yJcSH
> pPOL/8QrEUfZCiOnPwTy
> =GebV
> -END PGP SIGNATURE-
>

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

[Saúl Ibarra Corretgé]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 05/19/2014 09:19 PM, Fedor Indutny wrote:
> Introduce `record_start` field, indicating start of new TXT record,
> thus allowing to differentiate chunks in the same record, from a
> chunks in a different record.

Hey Fedor!

This patch changes the ABI, any chance this could be done by
concatenating the chunks and thus not requiring that extra field in
the ares_txt_reply struct?

- -- 
Saúl Ibarra Corretgé
bettercallsaghul.com

-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iQIcBAEBAgAGBQJTfZ5fAAoJEEEOVVOum8BZkPkQAI/0KXcb+jyv17pgWZvHzoyD
DesPcht2ZXh3l47aIdWrbQlVlgDqtqFmYT6eiMVoG03M/0C6uySXUGzqVz5LTFf1
EBiHGluGNbigHAfv8Rj+Cm++fDkuyGdDpwwqWhgytGGKHB/SCA8wlCEFr+eFgxEK
z2+sy0NKFGOZdZc1sholju8gYalUvTD3buKHgl0vKGcBTCIZOn12IiaySzeILHp/
FlBFl6v8ihqg8xEynhg4I9F/2AMWSjRXYjxyCB2lcc+9yAgy6QcSu6nimogstlM+
grlY3aMscVfEXAy4bmAFfKgxcbma6SX7tRDVNjEM2jZQITxL2+/MBEo00ny0YdDx
IYbUbjEBQSpNHkQtb9QQofgDfYHXCbsNjy5cNpIDHA2ENCySYzRkNAz2I/36p9g9
RigP8MYqYrgbZqjZsP47PSFlrmdhTGzWqss8dfLjJoQeIdmUnrLaUdU7h7mIGveq
IP3fbWhMoYJYnsvL3DP0Tvv/ChsitNuOkHIKP+xtOTZK/YMucJylxVNV99RvqvKZ
UY4SurNd60hN5N89+m92Yrgu8wokCj3Ya3XPmNd2Ufo18NrA1t7NQSPSo/h9frek
Sjaquzv2GmpnvLKO8dCJw0PwpIve1+T4weWk9kjstXbr8R55g5+4V3lbfI7yJcSH
pPOL/8QrEUfZCiOnPwTy
=GebV
-END PGP SIGNATURE-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Yang Tse]

Hi Jakub,

2009/11/22, Jakub Hrozek wrote:

>
> See attached patch prefix-structures-with-ares.patch
>
> [...]
>
> OK, I saw you did some changes already, so I hope I won't be stepping on
> your toe with this, but see the attached
> sync-man-pages-with-prototypes.patch patch.
>
> Hope this helps,

Certainly! Without your work this would have taken longer. Thanks a lot.

Cheers,
-- 
-=[Yang]=-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/20/2009 02:54 AM, Yang Tse wrote:
> 1)
> 
> In ares.h there's a comment mentioning that a couple of structs should
> be renamed to avoid name space pollution, prefixing ares_ to them.
> Obviously the change should be reflected elsewhere, code and man
> pages.
> 
> Changing the above would not require existing apps to be recompiled,
> so theoretically no ABI breakage here, but apps depending on these
> could need to be modified to match the new struct name when
> recompiled.
> 
> So I think it is a good moment to rise the issue and ask if someone is
> against this change. Or if we actually prefer it to happen right away.
> 

See attached patch prefix-structures-with-ares.patch

> 2)
> 
> Man pages should be revisited verifying that the data type of the
> prototypes match those present in ares.h, I remember once seeing at
> least one discrepancy. Don't really know if there are many
> discrepancies but I don't think so.

OK, I saw you did some changes already, so I hope I won't be stepping on
your toe with this, but see the attached
sync-man-pages-with-prototypes.patch patch.

Hope this helps,
Jakub

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAksJToAACgkQHsardTLnvCXLtwCgzv/upJYQe1ZN1VnhrDUGxAQx
OnkAoKuUVTaqybaK4DNZjuxlnUGYUy56
=WUa1
-END PGP SIGNATURE-
Index: ares.h
===
RCS file: /cvsroot/curl/curl/ares/ares.h,v
retrieving revision 1.70
diff -u -r1.70 ares.h
--- ares.h	20 Nov 2009 14:11:06 -	1.70
+++ ares.h	22 Nov 2009 13:57:41 -
@@ -416,15 +416,11 @@
   } _S6_un;
 };
 
-/*
- * TODO: the structs 'addrttl' and 'addr6ttl' really should get their names
- * prefixed with ares_ to keep them in our own "name space".
- */
-struct addrttl {
+struct ares_addrttl {
   struct in_addr ipaddr;
   intttl;
 };
-struct addr6ttl {
+struct ares_addr6ttl {
   struct ares_in6_addr ip6addr;
   int ttl;
 };
@@ -454,13 +450,13 @@
 CARES_EXTERN int ares_parse_a_reply(const unsigned char *abuf,
 int alen,
 struct hostent **host,
-struct addrttl *addrttls,
+struct ares_addrttl *addrttls,
 int *naddrttls);
 
 CARES_EXTERN int ares_parse__reply(const unsigned char *abuf,
int alen,
struct hostent **host,
-   struct addr6ttl *addrttls,
+   struct ares_addr6ttl *addrttls,
int *naddrttls);
 
 CARES_EXTERN int ares_parse_ptr_reply(const unsigned char *abuf,
Index: ares_parse_a_reply.3
===
RCS file: /cvsroot/curl/curl/ares/ares_parse_a_reply.3,v
retrieving revision 1.4
diff -u -r1.4 ares_parse_a_reply.3
--- ares_parse_a_reply.3	29 Oct 2009 09:06:42 -	1.4
+++ ares_parse_a_reply.3	22 Nov 2009 13:57:41 -
@@ -23,7 +23,7 @@
 .PP
 .B int ares_parse_a_reply(const unsigned char *\fIabuf\fP, int \fIalen\fP,
 .B 	struct hostent **\fIhost\fP,
-.B  struct addrttl *\fIaddrttls\fB, int *\fInaddrttls\fB);
+.B  struct ares_addrttl *\fIaddrttls\fB, int *\fInaddrttls\fB);
 .fi
 .SH DESCRIPTION
 The
@@ -31,7 +31,7 @@
 function parses the response to a query of type A into a
 .BR "struct hostent"
 and/or an array of
-.BR "struct addrttls" . 
+.BR "struct ares_addrttls" . 
 The parameters
 .I abuf
 and
@@ -51,7 +51,7 @@
 .IR naddrttls
 are both nonnull,
 then up to *naddrttls
-.BR "struct addrttl"
+.BR "struct ares_addrttl"
 records are stored in the array pointed to by addrttls,
 and then *naddrttls is set to the number of records so stored.
 Note that the memory for these records is supplied by the caller.
Index: ares_parse_a_reply.c
===
RCS file: /cvsroot/curl/curl/ares/ares_parse_a_reply.c,v
retrieving revision 1.19
diff -u -r1.19 ares_parse_a_reply.c
--- ares_parse_a_reply.c	2 Nov 2009 11:55:53 -	1.19
+++ ares_parse_a_reply.c	22 Nov 2009 13:57:41 -
@@ -54,7 +54,7 @@
 
 int ares_parse_a_reply(const unsigned char *abuf, int alen,
struct hostent **host,
-   struct addrttl *addrttls, int *naddrttls)
+   struct ares_addrttl *addrttls, int *naddrttls)
 {
   unsigned int qdcount, ancount;
   int status, i, rr_type, rr_class, rr_len, rr_ttl, naddrs;
@@ -157,7 +157,7 @@
 }
   if (naddrs < max_addr_ttls)
 {
-  struct addrttl * const at = &addrttls[naddrs];
+  struct ares_addrttl * const at = &addrttls[naddrs];
   if (aptr + sizeof(struct in_addr) > abuf + alen)
 

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Yang Tse]

Jakub and all,

ares_parse_srv_reply() and ares_parse_txt_reply() updated in CVS with
your API changes and also to make use of the new ares_free_data().

No man page yet for ares_free_data(), but comments in ares_data.c and
ares_data.h should give you nearly all the info that there will be in
the man page.

Jakub I hope you test these ares_parse_srv_reply() and
ares_parse_txt_reply()  thoroughly and see if everything is working as
expected.

Cheers,
-- 
-=[Yang]=-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Yang Tse]

2009/11/20, Jakub Hrozek wrote:

> is there anything I can help with, either with the generic
> free or just polishing the next release in general?

>From the general part I'm aware at least of two points right away.

1)

In ares.h there's a comment mentioning that a couple of structs should
be renamed to avoid name space pollution, prefixing ares_ to them.
Obviously the change should be reflected elsewhere, code and man
pages.

Changing the above would not require existing apps to be recompiled,
so theoretically no ABI breakage here, but apps depending on these
could need to be modified to match the new struct name when
recompiled.

So I think it is a good moment to rise the issue and ask if someone is
against this change. Or if we actually prefer it to happen right away.

2)

Man pages should be revisited verifying that the data type of the
prototypes match those present in ares.h, I remember once seeing at
least one discrepancy. Don't really know if there are many
discrepancies but I don't think so.

-- 
-=[Yang]=-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/09/2009 05:08 PM, Yang Tse wrote:
> Jakub,
> 
> I haven't forgotten you. Its simply I'm very short of time. You're the
> next on my list.
> 
> Cheers,

Thank you, Yang. I really appreciate it!

Just to shed some light on why I am hoping for a new c-ares release soon
and constantly bugging this list..there's an important Red Hat
Enterprise Linux deadline coming in roughly two weeks now, and we'd like
to ship an up-to-date version of c-ares (and also make the component
we're developing dependent on a recent version) for that deadline.

The contingency plan for me is to ship 1.6 + whole lotta patches..

Jakub
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAksF64IACgkQHsardTLnvCVWmgCgj9v3tMC1377WvPBS+wYxUPV2
2ZQAoMm2A6FyWhNzvdo/Izq30Fl1XcTy
=G8U2
-END PGP SIGNATURE-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Yang Tse]

Jakub,

I haven't forgotten you. Its simply I'm very short of time. You're the
next on my list.

Cheers,
-- 
-=[Yang]=-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/02/2009 07:59 PM, Yang Tse wrote:
> Hmm, I've started writing a theoretical comparison of both approaches,
> but I've deleted it all. Tomorrow most probably.I'm going to code my
> version, and afterwards I'll comment.

Hi,

is there anything I can help with, either with the generic free or just
polishing the next release in general?

Jakub
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkr4MnsACgkQHsardTLnvCWECwCg1vS3J944hrF0Lp+Pcb5De5pH
c4YAn19iTuWb9MN/j2HdYu7jkD4E5Yd7
=76yV
-END PGP SIGNATURE-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/02/2009 07:59 PM, Yang Tse wrote:
> It is obvious that the difference is directly storing an
> ares_[txt|srv]_reply struct in the 'ares private internal struct' or
> storing a pointer to a dynamically allocated ares_[txt|srv]_reply
> struct.
> 

Just in case it's not clear - the reason I opted for storing the struct
in the internal structure is way easier handling of the address to use
with offsetof during free.

Jakub
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrvMVMACgkQHsardTLnvCXW1ACghx6rVmtBm/6Es8wMD0kuH8qS
9MgAoNjKy7zbmKSHghHz5Y+92/dyJB7b
=1CzE
-END PGP SIGNATURE-

Re: [PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Yang Tse]

2009/11/2, Jakub Hrozek wrote:

> A patch is attached. I did factor in some of Yang's suggestions but so
> far it only works for SRV and TXT parsing routines..I'm not sure if what
> he posted (although a good plan!) is plan for the upcoming release or
> the next one and I did not want to touch existing (released) API much.

Let's first focus on the 'ares private internal struct' named
'ares_reply_private' in the patch vs the one named 'ares_data' in my
previous post. Copied both below for easier reference.

struct ares_reply_private {
  ares_datatype type;
  unsigned int  mark;   /* a private magic number */
  union {
  struct ares_txt_reply txt;
  struct ares_srv_reply srv;
  } data;
};

struct ares_data {
  ares_datatype type;
  unsigned int mark;
  void *data;
};

It is obvious that the difference is directly storing an
ares_[txt|srv]_reply struct in the 'ares private internal struct' or
storing a pointer to a dynamically allocated ares_[txt|srv]_reply
struct.

Hmm, I've started writing a theoretical comparison of both approaches,
but I've deleted it all. Tomorrow most probably.I'm going to code my
version, and afterwards I'll comment.

-- 
-=[Yang]=-

[PATCH] ares_free_reply (Was: Re: [PATCH] ares_parse_txt_reply)

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/31/2009 11:33 PM, Daniel Stenberg wrote:
> On Fri, 30 Oct 2009, Jakub Hrozek wrote:
> 
>> So, what about a slight change in the external structures:
>>
>> struct ares_srv_reply {
>>   
>>   struct ares_srv-reply *next;
>> }
>>
>> and have the private structure defined along the lines:
>>
>> struct ares_private {
>>int magic;
>>union {
>>   struct ares_srv_reply srv;
>>   struct ares_txt_reply txt;
>>} data;
>> };
> 
> Sure, that would work perfectly fine with me!
> 

A patch is attached. I did factor in some of Yang's suggestions but so
far it only works for SRV and TXT parsing routines..I'm not sure if what
he posted (although a good plan!) is plan for the upcoming release or
the next one and I did not want to touch existing (released) API much.

I'll also update manpages when/if this patch is deemed correct.

Comments welcome,
Jakub
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkru+SkACgkQHsardTLnvCUIAACaAgr5NJSEJNYOsC6twkZsjgHH
eIwAoOapTIS0guyNJQyOfmyymyY2R4MO
=gfM1
-END PGP SIGNATURE-
diff -up ./ares.h.mem ./ares.h
--- ./ares.h.mem	2009-11-02 15:34:21.0 +0100
+++ ./ares.h	2009-11-02 14:55:42.0 +0100
@@ -127,6 +127,7 @@ extern "C" {
 
 /* More error codes */
 #define ARES_ECANCELLED 24  /* introduced in 1.6.1 */
+#define ARES_EINVAL 25
 
 /* Flag values */
 #define ARES_FLAG_USEVC (1 << 0)
@@ -434,11 +435,15 @@ struct ares_srv_reply {
   unsigned short priority;
   unsigned short port;
   char *host;
+
+  struct ares_srv_reply *next;
 };
 
 struct ares_txt_reply {
   size_t length;  /* length excludes null termination */
   unsigned char *txt;
+
+  struct ares_txt_reply *next;
 };
 
 /*
@@ -474,18 +479,18 @@ CARES_EXTERN int ares_parse_ns_reply(con
 
 CARES_EXTERN int ares_parse_srv_reply(const unsigned char* abuf,
   int alen,
-  struct ares_srv_reply** srv_out,
-  int *nsrvreply);
+  struct ares_srv_reply** srv_out);
 
 CARES_EXTERN int ares_parse_txt_reply(const unsigned char* abuf,
   int alen,
-  struct ares_txt_reply** txt_out,
-  int *nsrvreply);
+  struct ares_txt_reply** txt_out);
 
 CARES_EXTERN void ares_free_string(void *str);
 
 CARES_EXTERN void ares_free_hostent(struct hostent *host);
 
+CARES_EXTERN int ares_free_reply(void *reply);
+
 CARES_EXTERN const char *ares_strerror(int code);
 
 #ifdef  __cplusplus
diff -up ./ares_parse_srv_reply.c.mem ./ares_parse_srv_reply.c
--- ./ares_parse_srv_reply.c.mem	2009-11-02 15:34:45.0 +0100
+++ ./ares_parse_srv_reply.c	2009-11-02 15:57:42.0 +0100
@@ -56,7 +56,7 @@
 
 int
 ares_parse_srv_reply (const unsigned char *abuf, int alen,
-  struct ares_srv_reply **srv_out, int *nsrvreply)
+  struct ares_srv_reply **srv_out)
 {
   unsigned int qdcount, ancount;
   const unsigned char *aptr;
@@ -64,13 +64,13 @@ ares_parse_srv_reply (const unsigned cha
   long len;
   char *hostname = NULL, *rr_name = NULL;
   struct ares_srv_reply *srv = NULL;
+  struct ares_srv_reply *last = NULL;
+  struct ares_srv_reply *head = NULL;
+  struct ares_reply_private *priv_reply = NULL;
 
   /* Set *srv_out to NULL for all failure cases. */
   *srv_out = NULL;
 
-  /* Same with *nsrvreply. */
-  *nsrvreply = 0;
-
   /* Give up if abuf doesn't have room for a header. */
   if (alen < HFIXEDSZ)
 return ARES_EBADRESP;
@@ -97,14 +97,31 @@ ares_parse_srv_reply (const unsigned cha
   aptr += len + QFIXEDSZ;
 
   /* Allocate ares_srv_reply array; ancount gives an upper bound */
-  srv = malloc ((ancount) * sizeof (struct ares_srv_reply));
-  if (!srv)
+  priv_reply = ares_alloc_reply (ARES_DATATYPE_SRV_REPLY);
+  if (!priv_reply)
 {
   free (hostname);
   return ARES_ENOMEM;
 }
+  head = &(priv_reply->data.srv);
+
+  last = head;
+  for (i = 0; i < ancount - 1; i++)
+{
+  srv = malloc (sizeof (struct ares_srv_reply));
+  if (!srv)
+{
+  free (hostname);
+  return ARES_ENOMEM;
+}
+  memset (srv, 0, sizeof (struct ares_srv_reply));
+
+  last->next = srv;
+  last = srv;
+}
 
   /* Examine each answer resource record (RR) in turn. */
+  srv = head;
   for (i = 0; i < (int) ancount; i++)
 {
   /* Decode the RR up to the data field. */
@@ -134,14 +151,14 @@ ares_parse_srv_reply (const unsigned cha
   break;
 }
 
-  srv[i].priority = ntohs (*((unsigned short *)aptr));
-  aptr += sizeof(unsigned short);
-  srv[i].weight = ntohs (*((unsigned short *)aptr));
-  aptr += sizeof(

Re: [PATCH] ares_parse_txt_reply

[Yang Tse]

Some additional brainstormin on the generic ares free function...

Would it be ok that the purpose of a c-ares external API
ares_freedata() function is to allow the calling application to free
memory resources that have been internally allocated by some c-ares
function and for which a pointer has already been returned to the
calling application?

It could be made generic enough as to be able to handle c-ares
structs, arrays of c-ares structs, strings, linked lists or whatever
we want. It could even handle the same for structs which are defined
elsewhere. In order to not break ABI, the only 'requisite' would be to
perfectly define which c-ares functions returning pointers to memory
allocated by c-ares require calling this ares_freedata() function.

The private data structure could be something like:

struct ares_data {
  ares_datatype type;
  unsigned int  mark;
  void *data;
};

Where ares_datatype could initially be for example something like:

typedef enum {
  ARES_DATATYPE_UNKNOWN = 1,
  ARES_DATATYPE_ADDR6TTL_PTR,
  ARES_DATATYPE_ADDRTTL_PTR,
  ARES_DATATYPE_HOSTENT_PTR,
  ARES_DATATYPE_OPTIONS_PTR,
  ARES_DATATYPE_SRV_REPLY_PTR,
  ARES_DATATYPE_TXT_REPLY_PTR,
  ARES_DATATYPE_LAST
} ares_datatype;

The pointer that c-ares functions using this would return would be the
'data' pointer of the ares_data struct. Later on This would be the
pointer that the calling application should pass to ares_freedata() to
free all the resources that c-ares had previously allocated.

The 'mark' member of the ares_data struct, would be a fixed signature
mark intended for the purpose of validating if a received pointer is a
pointer of something that actually was allocated by c-ares using the
ares_data struct 'mechanism'.

Comments?

-- 
-=[Yang]=-

Re: [PATCH] ares_parse_txt_reply

[Daniel Stenberg]

On Fri, 30 Oct 2009, Jakub Hrozek wrote:


So, what about a slight change in the external structures:

struct ares_srv_reply {
  
  struct ares_srv-reply *next;
}

and have the private structure defined along the lines:

struct ares_private {
   int magic;
   union {
  struct ares_srv_reply srv;
  struct ares_txt_reply txt;
   } data;
};


Sure, that would work perfectly fine with me!

--

 / daniel.haxx.se

Re: [PATCH] ares_parse_txt_reply

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/29/2009 11:55 AM, Daniel Stenberg wrote:
> On Thu, 29 Oct 2009, Jakub Hrozek wrote:
> 
>>> All it would take is that we make sure we include a hint in the
>>> returned data about what struct it is so that we can detect that in
>>> the free function and then do the correct cleanup.
>>
>> Well, from the point of view of the API user something along the lines
>> of ares_free() sure sounds good.
>>
>> How did you envision the implementation? Because some parsing
>> functions use standard structures (hostent), in order to expand the
>> returned data you either wrap the standard structures in ares specific
>> ones that would carry the hint or return the hint as another
>> parameter..both ways would change the API..
> 
> Right, those that return a "standard struct" we can't do much about. I
> really don't think we should add new APIs that return such structs but
> we can leave the exsisting as they are to not stir anything up.
> 
> For the ones that return an ares struct, we can either select to show
> the magic to the user or hide it from the users by doing it:
> 
>  struct ares_hidden {
>int magic;
>struct ares_external {
>  int public;
>}
>  }
> 
>  The ares_free() function would then use 'offsetof()' to figure out the
>  start address of the 'ares_hidden' struct, check what type that is
> embedded
>  and then call the appropriate internal free function.
> 

OK, I gave it some thought and I'm still not sure I understand -- if we
returned a single external structure, that would be easy, but since we
return an array, I'm afraid that would lead to ugliness in API.

Let's say we have a struct ares_private:

struct ares_private {
int magic;
void *data;
}

then, if the parsing function allocs struct ares_private *prv, you need
to offsetof starting at *address of data*, right?

prv == ((char *) &(prv->data) - offsetof(struct ares_private, data));

So, the parsing functions would need to return the address of data to
the user in order for the user to pass it back to the free function..and
now the user must deal with dereferencing, etc...this seems ugly to me..

So, what about a slight change in the external structures:

struct ares_srv_reply {
   
   struct ares_srv-reply *next;
}

and have the private structure defined along the lines:

struct ares_private {
int magic;
union {
   struct ares_srv_reply srv;
   struct ares_txt_reply txt;
} data;
};

Sorry for such a long braindump on a ML, I'd just like to make sure
we're on the same page and avoid coding something that would be thown
away anyway :-)

Jakub
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrrBEMACgkQHsardTLnvCWoXACbBT6yKbI0vCtWGqQN3cU2+w5G
nJcAoIm3mJNghOz43vDEwoeXIGSFuaS2
=Nc43
-END PGP SIGNATURE-

Re: [PATCH] ares_parse_txt_reply

[Yang Tse]

Hey,

struct ares_txt_reply {
  unsigned int  length;
  unsigned char *txt;
};

I would really like to change the 'length' data type to 'unsigned
long', or even more properly to 'size_t'. We can not be defensive
enough about what an evil DNS server might send back.


-- 
-=[Yang]=-

Re: [PATCH] ares_parse_txt_reply

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/29/2009 10:30 PM, Daniel Stenberg wrote:
> On Thu, 29 Oct 2009, Yang Tse wrote:
> 
>> Just a heads-up on this issue, CVS c-ares fails to compile for more
>> than twelve hours now. Are there any plans to fix/revert this before
>> daily snapshot?
> 
> That's related to the free function subject, as the
> ares_parse_txt_reply() hasn't been added to CVS and thus the build fails
> (since ares_parse_txt_reply() uses it).
> 
> It is my fault really who stopped to discuss this issue in the middle
> after having applied parts of Jakub's work. We'll work it out soon enough!
> 

Yeah, sorry, I didn't have the time to code that stuff up today. I think
I should have that ready for review tomorrow.

I did not spot the build failed b/c I still had my original free
function in the tree.

Jakub
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrqHQEACgkQHsardTLnvCXzswCgz3HSDZ8BS1xzU6rWC/LCCev6
Vp0An0Pqi+PsvCLoBFHxmpWRlNAa04nY
=8X6c
-END PGP SIGNATURE-

Re: [PATCH] ares_parse_txt_reply

[Yang Tse]

2009/10/29, Daniel Stenberg wrote:

> That's related to the free function subject, as the ares_parse_txt_reply()
> hasn't been added to CVS and thus the build fails (since
> ares_parse_txt_reply() uses it).

I know. I've seen it. and after reading the thread was aware that it
was work in progress and it wasn't clear to me which way was the
definitive one to take.

> [...] We'll work it out soon enough!

Thanks! I simply needed autobuilds working to verify correctness, and
adjust last changes to configure. But in any case it can wait, there's
always something else to do. ;-)

Cheers,
-- 
-=[Yang]=-

Re: [PATCH] ares_parse_txt_reply

[Daniel Stenberg]

On Thu, 29 Oct 2009, Yang Tse wrote:

Just a heads-up on this issue, CVS c-ares fails to compile for more than 
twelve hours now. Are there any plans to fix/revert this before daily 
snapshot?


That's related to the free function subject, as the ares_parse_txt_reply() 
hasn't been added to CVS and thus the build fails (since 
ares_parse_txt_reply() uses it).


It is my fault really who stopped to discuss this issue in the middle after 
having applied parts of Jakub's work. We'll work it out soon enough!


--

 / daniel.haxx.se

Re: [PATCH] ares_parse_txt_reply

[Yang Tse]

Hi Daniel and Jakub,

Just a heads-up on this issue, CVS c-ares fails to compile for more
than twelve hours now. Are there any plans to fix/revert this before
daily snapshot?

Cheers,
-- 
-=[Yang]=-

Re: [PATCH] ares_parse_txt_reply

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/29/2009 11:55 AM, Daniel Stenberg wrote:
> Right, those that return a "standard struct" we can't do much about. I
> really don't think we should add new APIs that return such structs but
> we can leave the exsisting as they are to not stir anything up.
> 
> For the ones that return an ares struct, we can either select to show
> the magic to the user or hide it from the users by doing it:
> 
>  struct ares_hidden {
>int magic;
>struct ares_external {
>  int public;
>}
>  }
> 
>  The ares_free() function would then use 'offsetof()' to figure out the
>  start address of the 'ares_hidden' struct, check what type that is
> embedded
>  and then call the appropriate internal free function.
> 
>  And when we return a pointer to this struct over the API, we just return a
>  pointer to the external part.
> 

I would prefer this approach as IMHO no magic should be visible to the user.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrplVsACgkQHsardTLnvCVwqwCgi+Z/nYG/UcSNhn4RpFTX9M0N
3HQAoLhoB0JC5mlUa/G/vRxQOe6IsS3C
=MgN5
-END PGP SIGNATURE-

Re: [PATCH] ares_parse_txt_reply

[Daniel Stenberg]

On Thu, 29 Oct 2009, Jakub Hrozek wrote:

All it would take is that we make sure we include a hint in the returned 
data about what struct it is so that we can detect that in the free 
function and then do the correct cleanup.


Well, from the point of view of the API user something along the lines of 
ares_free() sure sounds good.


How did you envision the implementation? Because some parsing functions use 
standard structures (hostent), in order to expand the returned data you 
either wrap the standard structures in ares specific ones that would carry 
the hint or return the hint as another parameter..both ways would change the 
API..


Right, those that return a "standard struct" we can't do much about. I really 
don't think we should add new APIs that return such structs but we can leave 
the exsisting as they are to not stir anything up.


For the ones that return an ares struct, we can either select to show the 
magic to the user or hide it from the users by doing it:


 struct ares_hidden {
   int magic;
   struct ares_external {
 int public;
   }
 }

 The ares_free() function would then use 'offsetof()' to figure out the
 start address of the 'ares_hidden' struct, check what type that is embedded
 and then call the appropriate internal free function.

 And when we return a pointer to this struct over the API, we just return a
 pointer to the external part.

 Or we let the magic get seen by the user :

  struct ares_external {
int hidden; /* c-ares internal variable, don't touch! */
int public;
  }

 I think I prefer the first approach. Mostly because the second approach makes
the magic visible in the public struct and then we need to document it and so
on and it makes it less nice.

--

 / daniel.haxx.se

Re: [PATCH] ares_parse_txt_reply

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

> All it would take is that we make sure we include a hint in the returned
> data about what struct it is so that we can detect that in the free
> function and then do the correct cleanup.
> 

Well, from the point of view of the API user something along the lines
of ares_free() sure sounds good.

How did you envision the implementation? Because some parsing functions
use standard structures (hostent), in order to expand the returned data
you either wrap the standard structures in ares specific ones that would
carry the hint or return the hint as another parameter..both ways would
change the API..

Jakub

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrpbeUACgkQHsardTLnvCVXOwCfXpC5r+wmSdmKuPrzC2V7zns4
Nq4AnRaA2l5Py+dsLC6UEsEmbNPv2oeq
=RY/R
-END PGP SIGNATURE-

Re: [PATCH] ares_parse_txt_reply

[Daniel Stenberg]

On Wed, 28 Oct 2009, Jakub Hrozek wrote:

Yes, that is a bug. I think the best solution might be to provide a free 
function and use it both internally in cases like this and provide it 
externally.


Right, we have that problem with these functions allocating memory that is 
returned so we need to provide a function that can free that memory (we cannot 
assume the app can use free() to free memory returned by our lib).


I remember we got a ares_free_txt_reply() function before (that isn't in CVS 
and that isn't properly referenced to from the ares_parse_txt_reply man page 
etc). Do you think it would make sense that we perhaps instead try to unify 
how we deal with allocated memory handed out in the API so that we can have a 
single function that can do the free? I really don't like that we're 
progressing towards adding a new free function for every new parse function.


All it would take is that we make sure we include a hint in the returned data 
about what struct it is so that we can detect that in the free function and 
then do the correct cleanup.


Thoughts?

--

 / daniel.haxx.se

Re: [PATCH] ares_parse_txt_reply

[Daniel Stenberg]

On Wed, 28 Oct 2009, Jakub Hrozek wrote:


Thank you for the review. A new patch is attached.


Thanks, applied and committed. I renamed the srv_reply struct too while I was 
at it.


--

 / daniel.haxx.se

Re: [PATCH] ares_parse_txt_reply

[Jakub Hrozek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/27/2009 07:26 PM, Daniel Stenberg wrote:
> I gave it a quick glance just now and I spotted two things:
> 
> 1) 'struct txt_reply' is a global struct and should thus be with ares_
> prefix
>like ares_txt_reply or similar.
> 

Renamed. I should probably rename 'struct srv_reply' to 'struct
ares_srv_reply' in a followup patch as well..?

> 2) in the loop I spotted a malloc() that does break on a NULL return, but
>there's no free()ing done on previous successful malloc()s and thus this
>function risks leaking memory. (I realize it is an edge case, but
> still...)
> 

Yes, that is a bug. I think the best solution might be to provide a free
function and use it both internally in cases like this and provide it
externally.

Thank you for the review. A new patch is attached.

Jakub
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrosi0ACgkQHsardTLnvCVO9QCg6KplNxROwRye5HMozh0MOyOp
Oh0AnRX7FtiQjuSVvnQ13QzBZp1p0VZK
=xxSW
-END PGP SIGNATURE-
diff -up ./ares_free_txt_reply.3.txt ./ares_free_txt_reply.3
--- ./ares_free_txt_reply.3.txt	2009-10-28 17:55:08.0 +0100
+++ ./ares_free_txt_reply.3	2009-10-28 18:03:00.0 +0100
@@ -0,0 +1,41 @@
+.\"
+.\" Copyright 1998 by the Massachusetts Institute of Technology.
+.\"
+.\" Permission to use, copy, modify, and distribute this
+.\" software and its documentation for any purpose and without
+.\" fee is hereby granted, provided that the above copyright
+.\" notice appear in all copies and that both that copyright
+.\" notice and this permission notice appear in supporting
+.\" documentation, and that the name of M.I.T. not be used in
+.\" advertising or publicity pertaining to distribution of the
+.\" software without specific, written prior permission.
+.\" M.I.T. makes no representations about the suitability of
+.\" this software for any purpose.  It is provided "as is"
+.\" without express or implied warranty.
+.\"
+.TH ARES_FREE_TXT_REPLY 3 "28 October 2009"
+.SH NAME
+ares_free_txt_reply \- Free a reply from ares_parse_txt_reply
+.SH SYNOPSIS
+.nf
+.B #include 
+.PP
+.B int ares_free_txt_reply(struct ares_txt_reply *\fIreply\fP, int \fIntxtreply\fP);
+.fi
+.SH DESCRIPTION
+The
+.B ares_free_txt_reply
+function frees the response to a query of type TXT previously returned from a previous
+.B ares_parse_txt_reply
+call.
+.PP
+The data to be freed is stored in the variable
+.IR reply .
+The number of responses is stored into the variable
+.IR ntxtreply .
+.fi
+.SH SEE ALSO
+.BR ares_parse_txt_reply (3)
+.SH AUTHOR
+Written by Jakub Hrozek 
+
diff -up ./ares_free_txt_reply.c.txt ./ares_free_txt_reply.c
--- ./ares_free_txt_reply.c.txt	2009-10-28 17:55:13.0 +0100
+++ ./ares_free_txt_reply.c	2009-10-28 18:54:52.0 +0100
@@ -0,0 +1,29 @@
+/* Copyright (C) 2009 Jakub Hrozek 
+*
+* Permission to use, copy, modify, and distribute this
+* software and its documentation for any purpose and without
+* fee is hereby granted, provided that the above copyright
+* notice appear in all copies and that both that copyright
+* notice and this permission notice appear in supporting
+* documentation, and that the name of M.I.T. not be used in
+* advertising or publicity pertaining to distribution of the
+* software without specific, written prior permission.
+* M.I.T. makes no representations about the suitability of
+* this software for any purpose.  It is provided "as is"
+* without express or implied warranty.
+*/
+
+#include 
+#include "ares.h"
+
+void
+ares_free_txt_reply (struct ares_txt_reply *reply, int ntxtreply)
+{
+  int i;
+
+  for (i = 0; i < ntxtreply; i++)
+{
+  free (reply[i].txt);
+}
+  free (reply);
+}
diff -up ./ares.h.txt ./ares.h
--- ./ares.h.txt	2009-10-27 15:32:07.0 +0100
+++ ./ares.h	2009-10-28 15:36:10.0 +0100
@@ -331,6 +331,11 @@ struct srv_reply {
   char *host;
 };
 
+struct ares_txt_reply {
+  unsigned int  length;
+  unsigned char *txt;
+};
+
 /*
 ** Parse the buffer, starting at *abuf and of length alen bytes, previously
 ** obtained from an ares_search call.  Put the results in *host, if nonnull.
@@ -350,8 +355,11 @@ int ares_parse_ns_reply(const unsigned c
 struct hostent **host);
 int ares_parse_srv_reply(const unsigned char* abuf, int alen,
  struct srv_reply** srv_out, int *nsrvreply);
+int ares_parse_txt_reply(const unsigned char* abuf, int alen,
+ struct ares_txt_reply **txt_out, int *ntxtreply);
 void ares_free_string(void *str);
 void ares_free_hostent(struct hostent *host);
+void ares_free_txt_reply(struct ares_txt_reply *reply, int ntxtreply);
 const char *ares_strerror(int code);
 
 #ifdef  __cplusplus
diff -up ./ares_parse_txt_reply.3.txt ./ares_parse_txt_reply.3
--- ./ares_parse_txt_reply.3.txt	2009-10-27 18:23:32.0 +0100
+++ ./ares_parse_txt_reply.3	

Re: [PATCH] ares_parse_txt_reply

[Daniel Stenberg]

On Tue, 27 Oct 2009, Jakub Hrozek wrote:

I tested that apart from simple records and multiple records per host, it 
also handles multiple substrings in the TXT record (used to make the whole 
string>255 bytes, configured as 'host IN TXT ( "Foo" "Bar" )' in Bind).


Comments or reviews are mostly welcome!


I gave it a quick glance just now and I spotted two things:

1) 'struct txt_reply' is a global struct and should thus be with ares_ prefix
   like ares_txt_reply or similar.

2) in the loop I spotted a malloc() that does break on a NULL return, but
   there's no free()ing done on previous successful malloc()s and thus this
   function risks leaking memory. (I realize it is an edge case, but still...)

--

 / daniel.haxx.se