[PATCH] ares_parse_srv_reply

Hello,

we are strongly considering using c-ares for a project we are working on
[1]. One thing that, from our perspective, was missing from c-ares was
the ability to parse SRV records. I scanned the archives briefly and
found one post dated 2004 [2] that said you wouldn't be opposed to
adding such functionality.

The attached patch adds a ares_parse_srv_reply function. I look at other
ares_parse_XXX_reply functions for inspiration, but as this is my first
patch for the project, I guess there will be some comments as per code
quality or formatting standards - of course I'm willing to fix these.

Thank you,
Jakub Hrozek

[1] https://fedorahosted.org/sssd/
[2] http://curl.haxx.se/mail/lib-2004-03/0302.html

diff -up ./ares.h.srv ./ares.h
--- ./ares.h.srv	2009-07-01 18:05:38.000000000 +0200
+++ ./ares.h	2009-07-01 18:07:26.000000000 +0200
 <at>  <at>  -321,6 +321,13  <at>  <at>  struct addr6ttl {
   int             ttl;
 };

+struct srv_reply {
+    int weight;

Re: [PATCH] ares_parse_srv_reply

Jakub Hrozek wrote:
> +struct srv_reply {
> +    int weight;
> +    int priority;
> +    int port;
> +    char *host;
> +};
[..]
> +#define SRV_PRIORITY_SIZE   2
> +#define SRV_WEIGHT_SIZE     2
> +#define SRV_PORT_SIZE       2
[..]
> +          memcpy ((void *) &srv[i].priority, aptr, SRV_PRIORITY_SIZE);
> +          aptr += SRV_PRIORITY_SIZE;
> +          memcpy ((void *) &srv[i].weight, aptr, SRV_WEIGHT_SIZE);
> +          aptr += SRV_WEIGHT_SIZE;
> +          memcpy ((void *) &srv[i].port, aptr, SRV_PORT_SIZE);
> +          aptr += SRV_PORT_SIZE;
Hi,

This looks like it will break badly if those fields aren't 2 byte large (they
aren't) and if the mem isn't initialized to zero (it isn't, allocated via malloc
which doesn't initialize mem).
Oh and I'd expect this to break badly on some endian where it writes to the high
bytes of the "int"s.

Feel free to correct me if I'm wrong.

Cheers,
Uli

Re: [PATCH] ares_parse_srv_reply

Hello,
thanks for reviewing the patch!

On 07/01/2009 08:06 PM, Uli wrote:
> Hi,
> 
> This looks like it will break badly if those fields aren't 2 byte large (they
> aren't)

My main source of documentation when writing the function was RFC 2782
[1], which specifically says "This is a 16 bit unsigned integer in
network byte order." in description of the port, weight and priority
fields on pages 2 and 3 of the RFC. I also checked the source of ISC
Bind 9.6.1, the structure for SRV records is declared in file
lib/dns/rdata/in_1/srv_33.h and the fields are declared as isc_uint16_t
there.

Actually, I changed the weight, priority and port members of struct
srv_reply to be u_int16_t as they were plain int in the previous patch.
New version of the patch is attached.

(Related question: would you prefer using u_int_XX_t from sys/types.h
which is unconditionally included in ares.h or rather ISO C99-compliant
uint_XX_t types from stdint.h?)

> and if the mem isn't initialized to zero (it isn't, allocated via malloc
> which doesn't initialize mem).

I don't quite understand why is this a problem. The uninitialized value

Re: [PATCH] ares_parse_srv_reply

Jakub Hrozek wrote:
> Hello,
> thanks for reviewing the patch!
> 
> On 07/01/2009 08:06 PM, Uli wrote:
>> Hi,
> 
>> This looks like it will break badly if those fields aren't 2 byte large (they
>> aren't)
> 
> My main source of documentation when writing the function was RFC 2782
> [1], which specifically says "This is a 16 bit unsigned integer in
> network byte order." in description of the port, weight and priority
> fields on pages 2 and 3 of the RFC. I also checked the source of ISC
> Bind 9.6.1, the structure for SRV records is declared in file
> lib/dns/rdata/in_1/srv_33.h and the fields are declared as isc_uint16_t
> there.
> 
> Actually, I changed the weight, priority and port members of struct
> srv_reply to be u_int16_t as they were plain int in the previous patch.
> New version of the patch is attached.

Ok, this should fix my doubts. :)

> (Related question: would you prefer using u_int_XX_t from sys/types.h
> which is unconditionally included in ares.h or rather ISO C99-compliant
> uint_XX_t types from stdint.h?)

Don't ask me, I'm not a regular here.

Re: [PATCH] ares_parse_srv_reply

On 07/01/2009 06:21 PM, Jakub Hrozek wrote:
> I guess there will be some comments as per code
> quality or formatting standards - of course I'm willing to fix these.
> 

Just a note, I will be on vacation next week, so if there are any
comments or code reviews related to this patch - I'm not ignoring them
and I'll get back to them in the week starting Jul 13.

Cheers,
Jakub

gertting error T_NAPTR undeclared

Hi All,
I have configured c-ares for AIX 52 with the following option:
./configure --enable-static=yes

But when I ran make I got the following error:

       if gcc -DHAVE_CONFIG_H  -I.  -I.     -g -O2 -MT adig.o -MD -MP
-MF ".deps/adig.Tpo" -c -o adig.o adig.c;  then mv -f ".deps/adig.Tpo"
".deps/adig.Po"; else rm -f ".deps/adig.Tpo"; exit 1; fi
adig.c:119: error: `T_NAPTR' undeclared here (not in a function)
adig.c:119: error: initializer element is not constant
adig.c:119: error: (near initialization for `types[33].value')
adig.c:119: error: initializer element is not constant
adig.c:119: error: (near initialization for `types[33]')
adig.c:120: error: initializer element is not constant
adig.c:120: error: (near initialization for `types[34]')
adig.c: In function `display_rr':
adig.c:615: error: `T_NAPTR' undeclared (first use in this function)
adig.c:615: error: (Each undeclared identifier is reported only once
adig.c:615: error: for each function it appears in.)
make: 1254-004 The error code from the last command is 1.

I checked config.log and the error is in :

/usr/include/arpa/nameser_compat.h:244: error: conflicting types for `HEADER'
/usr/include/arpa/onameser_compat.h:265: error: previous declaration of `HEADER'

I am using gcc 3.3.2
bash-3.00# gcc -v
Reading specs from /opt/freeware/lib/gcc-lib/powerpc-ibm-aix5.2.0.0/3.3.2/specs

Re: gertting error T_NAPTR undeclared

On Mon, 6 Jul 2009, Vivek Katakam wrote:

> I checked config.log and the error is in :
>
> /usr/include/arpa/nameser_compat.h:244: error: conflicting types for `HEADER'
> /usr/include/arpa/onameser_compat.h:265: error: previous declaration of `HEADER'

But how come the latter header file is included? We only explicitly include 
the first one so I figure the second one is a mistake/oversight/bug.

--

-- 

  / daniel.haxx.se

Re: gertting error T_NAPTR undeclared

Hi Daniel,

In the following compilation,
gcc -DHAVE_CONFIG_H -I.    -g -O2 -MT adig.o -MD -MP -MF
".deps/adig.Tpo" -c -o adig.o adig.c

To check this, i have temporaily renamed the file
/usr/include/arpa/onameser_compat.h as
/usr/include/arpa/onameser_compat. This time i got this error:

In file included from adig.c:29:
/usr/include/arpa/nameser.h:645:34: arpa/onameser_compat.h: No such
file or directory

The following are the lines in the /usr/include/arpa/nameser.h:
  +640  #ifdef _USE_IRS
  +641  #ifdef BIND_4_COMPAT
  +642  #include <arpa/nameser_compat.h>
  +643  #endif /* BIND_4_COMPAT */
  +644  #else /* _USE_IRS */
  +645  #include <arpa/onameser_compat.h>
  +646  #endif /* _USE_IRS */
  +647
  +648  #endif /* !_ARPA_NAMESER_H_ */

Thanks and Regards,
Vivek

On Mon, Jul 6, 2009 at 2:56 AM, Daniel Stenberg<daniel@...> wrote:
> On Mon, 6 Jul 2009, Vivek Katakam wrote:

Re: gertting error T_NAPTR undeclared

On Sat, 11 Jul 2009, Vivek Katakam wrote:

Please don't top-post. It confuses me greatly.

> In the following compilation,
> gcc -DHAVE_CONFIG_H -I.    -g -O2 -MT adig.o -MD -MP -MF
> ".deps/adig.Tpo" -c -o adig.o adig.c

If your problem is only in the adig compile you can safely ignore that. It's 
just a demo/example _using_ c-ares that isn't necessary for c-ares to work.

--

-- 

  / daniel.haxx.se

Re: gertting error T_NAPTR undeclared

Hi Daniel,
Thanks. I got the compilation.

Thanks and Regards,
Vivek

> Please don't top-post. It confuses me greatly.
>
> In the following compilation,
> gcc -DHAVE_CONFIG_H -I.    -g -O2 -MT adig.o -MD -MP -MF
> ".deps/adig.Tpo" -c -o adig.o adig.c
>
> If your problem is only in the adig compile you can safely ignore that. It's
> just a demo/example _using_ c-ares that isn't necessary for c-ares to work.
>
> --

>> Hi Daniel,
>> In the following compilation,
>> gcc -DHAVE_CONFIG_H -I. -g -O2 -MT adig.o -MD -MP -MF
>> ".deps/adig.Tpo" -c -o adig.o adig.c

>> To check this, i have temporaily renamed the file
>> /usr/include/arpa/onameser_compat.h as
>> /usr/include/arpa/onameser_compat .This time i got this error:

>>  In file included from adig.c:29:
>>  /usr/include/arpa/nameser.h:645:34: arpa/onameser_compat.h: No such
>>  file or directory