Date: Sat, 8 Nov 2008 16:10:23 +0100 From: Luigi Rizzo <rizzo@iet.unipi.it> To: Hajimu UMEMOTO <ume@freebsd.org> Cc: net@freebsd.org Subject: Re: Two copies of resolver routines in libc ? Message-ID: <20081108151023.GA72747@onelab2.iet.unipi.it> In-Reply-To: <yge3ai2r4pz.wl%ume@mahoroba.org> References: <20081106192103.GA46651@onelab2.iet.unipi.it> <yge3ai2r4pz.wl%ume@mahoroba.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Sat, Nov 08, 2008 at 05:39:52PM +0900, Hajimu UMEMOTO wrote:
> Hi,
>
> >>>>> On Thu, 6 Nov 2008 20:21:03 +0100
> >>>>> Luigi Rizzo <rizzo@iet.unipi.it> said:
>
> rizzo> While looking for a workaround (attached, read later), i noticed
> rizzo> that libc has two versions of the resolver routines: one is in
...
> rizzo> If we are lucky, this is just replicated code.
>
> No, the resolver functions in getaddrinfo.c has some addition of
> functionality. It was done for solving the query order problem.
...
> When we tried to solve the query order problem, we decided to have a
> modification code of the resolver into getaddrinfo.c. Because:
>
> - Hide the internal functions from outside of libc.
> - Don't change the resolver as possible to ease further merge from
> ISC BIND.
>
> Since we have the symbol versioning facility, we can hide the libc
> internal functions from outside of libc, these days. So, it may
> better to merge the two. I attached the proposed patch in this mail.
> Please review it.
hi,
thanks for the answer and the effort for merging back the two copies.
I am looking at your proposed patch but i am not sure i understand it fully,
so i would be grateful if you could explain me a few things (btw i
am attaching a version of your patch constructed with 'diff -ubwp'
and couple of formatting changes to remove whitespace differences;
this should make the actual changes more evident):
1. i apologize for my ignorance, but i suppose there is a binding
between __res_nsearchN() and res_nsearchN(), and the same
for all function with and without the two leading __ . Where
is this binding established, as i don't see anything in the
diff that you sent.
2. res_*() now become wrappers around the newly introduced res_*N()
functions. While this approach removes the duplication, it does
not address the "ease further merge from ISC BIND" case, which
definitely sounded as a valid concern.
So i wonder, what is it that prevents you from acting the other
way around, i.e. build the res_*N() around the existing res_*()
functions ? This way the original ISC code would really be
unmodified and perhaps the change would be even more confined.
Does this constraint apply to all three functions (res_query, res_search,
res_nquerydomain) or only for a subset of them ?
From what i can tell, at least in res_nquery()
the only significant change seems to be the following
+ if (n > anslen)
+ hp->rcode = FORMERR; /* XXX not very informative */
other than that it seems perfectly fine to implement res_nqueryN()
as a wrapper around the original res_nquery().
For the other two functions
and the other one (trspping TRYAGAIN) seems perfectly suitable to
be implemented in
2. i don't completely understand what is the additional functionality
in the resolver functions. You mention a 'query order problem'
but i don't exactly understand what it is and how the functional
change is implemented.
thanks
luigi
[-- Attachment #2 --]
Index: resolv/res_private.h
===================================================================
RCS file: /home/ncvs/src/lib/libc/resolv/res_private.h,v
retrieving revision 1.1.1.2
diff -u -w -b -p -r1.1.1.2 res_private.h
--- resolv/res_private.h 3 Jun 2007 17:02:28 -0000 1.1.1.2
+++ resolv/res_private.h 8 Nov 2008 14:29:00 -0000
@@ -14,9 +14,21 @@ struct __res_state_ext {
char nsuffix2[64];
};
+struct res_target {
+ struct res_target *next;
+ const char *name; /* domain name */
+ int qclass, qtype; /* class and type of query */
+ u_char *answer; /* buffer to put answer */
+ int anslen; /* size of answer buffer */
+ int n; /* result length */
+};
+
extern int
res_ourserver_p(const res_state statp, const struct sockaddr *sa);
+extern int
+__res_nsearchN(res_state, const char *, struct res_target *);
+
#endif
/*! \file */
Index: resolv/res_query.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/resolv/res_query.c,v
retrieving revision 1.5
diff -u -w -b -p -r1.5 res_query.c
--- resolv/res_query.c 3 Jun 2007 17:20:27 -0000 1.5
+++ resolv/res_query.c 8 Nov 2008 14:31:50 -0000
@@ -87,6 +87,8 @@ __FBSDID("$FreeBSD: src/lib/libc/resolv/
#include <unistd.h>
#include "port_after.h"
+#include "res_private.h"
+
/* Options. Leave them on. */
#define DEBUG
@@ -96,6 +98,10 @@ __FBSDID("$FreeBSD: src/lib/libc/resolv/
#define MAXPACKET 1024
#endif
+static int res_nqueryN(res_state, const char *, struct res_target *);
+static int res_nquerydomainN(res_state, const char *, const char *,
+ struct res_target *);
+
/*%
* Formulate a normal query, send, and await answer.
* Returned answer is placed in supplied buffer "answer".
@@ -113,10 +119,45 @@ res_nquery(res_state statp,
u_char *answer, /*%< buffer to put answer */
int anslen) /*%< size of answer buffer */
{
+ struct res_target q;
+ int n;
+
+ memset(&q, 0, sizeof(q));
+ q.name = name;
+ q.qclass = class;
+ q.qtype = type;
+ q.answer = answer;
+ q.anslen = anslen;
+ n = res_nqueryN(statp, name, &q);
+ return ((n < 0) ? n : q.n);
+}
+
+static int
+res_nqueryN(res_state statp, const char *name, struct res_target *target)
+{
u_char buf[MAXPACKET];
- HEADER *hp = (HEADER *) answer;
+ HEADER *hp;
int n;
u_int oflags;
+ struct res_target *t;
+ int rcode;
+ int ancount;
+
+ rcode = NOERROR;
+ ancount = 0;
+
+ for (t = target; t; t = t->next) {
+ int class, type;
+ u_char *answer;
+ int anslen;
+
+ hp = (HEADER *)(void *) t->answer;
+
+ /* make it easier... */
+ class = t->qclass;
+ type = t->qtype;
+ answer = t->answer;
+ anslen = t->anslen;
oflags = statp->_flags;
@@ -154,15 +195,18 @@ again:
goto again;
}
#endif
+ rcode = hp->rcode; /* record most recent error */
#ifdef DEBUG
if (statp->options & RES_DEBUG)
printf(";; res_query: send error\n");
#endif
- RES_SET_H_ERRNO(statp, TRY_AGAIN);
- return (n);
+ continue;
}
+ if (n > anslen)
+ hp->rcode = FORMERR; /* XXX not very informative */
if (hp->rcode != NOERROR || ntohs(hp->ancount) == 0) {
+ rcode = hp->rcode; /* record most recent error */
#ifdef DEBUG
if (statp->options & RES_DEBUG)
printf(";; rcode = (%s), counts = an:%d ns:%d ar:%d\n",
@@ -171,7 +215,16 @@ again:
ntohs(hp->nscount),
ntohs(hp->arcount));
#endif
- switch (hp->rcode) {
+ continue;
+ }
+
+ ancount += ntohs(hp->ancount);
+
+ t->n = n;
+ }
+
+ if (ancount == 0) {
+ switch (rcode) {
case NXDOMAIN:
RES_SET_H_ERRNO(statp, HOST_NOT_FOUND);
break;
@@ -190,7 +243,7 @@ again:
}
return (-1);
}
- return (n);
+ return (ancount);
}
/*%
@@ -206,8 +259,24 @@ res_nsearch(res_state statp,
u_char *answer, /*%< buffer to put answer */
int anslen) /*%< size of answer */
{
+ struct res_target q;
+ int n;
+
+ memset(&q, 0, sizeof(q));
+ q.name = name;
+ q.qclass = class;
+ q.qtype = type;
+ q.answer = answer;
+ q.anslen = anslen;
+ n = __res_nsearchN(statp, name, &q);
+ return ((n < 0) ? n : q.n);
+}
+
+int
+__res_nsearchN(res_state statp, const char *name, struct res_target *target)
+{
const char *cp, * const *domain;
- HEADER *hp = (HEADER *) answer;
+ HEADER *hp = (HEADER *)(void *) target->answer;
char tmp[NS_MAXDNAME];
u_int dots;
int trailing_dot, ret, saved_herrno;
@@ -226,7 +295,7 @@ res_nsearch(res_state statp,
/* If there aren't any dots, it could be a user-level alias. */
if (!dots && (cp = res_hostalias(statp, name, tmp, sizeof tmp))!= NULL)
- return (res_nquery(statp, cp, class, type, answer, anslen));
+ return (res_nqueryN(statp, cp, target));
/*
* If there are enough dots in the name, let's just give it a
@@ -235,8 +304,7 @@ res_nsearch(res_state statp,
*/
saved_herrno = -1;
if (dots >= statp->ndots || trailing_dot) {
- ret = res_nquerydomain(statp, name, NULL, class, type,
- answer, anslen);
+ ret = res_nquerydomainN(statp, name, NULL, target);
if (ret > 0 || trailing_dot)
return (ret);
if (errno == ECONNREFUSED) {
@@ -280,9 +348,7 @@ res_nsearch(res_state statp,
if (root_on_list && tried_as_is)
continue;
- ret = res_nquerydomain(statp, name, *domain,
- class, type,
- answer, anslen);
+ ret = res_nquerydomainN(statp, name, *domain, target);
if (ret > 0)
return (ret);
@@ -366,8 +432,7 @@ res_nsearch(res_state statp,
*/
if ((dots || !searched || (statp->options & RES_NOTLDQUERY) == 0U) &&
!(tried_as_is || root_on_list)) {
- ret = res_nquerydomain(statp, name, NULL, class, type,
- answer, anslen);
+ ret = res_nquerydomainN(statp, name, NULL, target);
if (ret > 0)
return (ret);
}
@@ -401,6 +466,23 @@ res_nquerydomain(res_state statp,
u_char *answer, /*%< buffer to put answer */
int anslen) /*%< size of answer */
{
+ struct res_target q;
+ int n;
+
+ memset(&q, 0, sizeof(q));
+ q.name = name;
+ q.qclass = class;
+ q.qtype = type;
+ q.answer = answer;
+ q.anslen = anslen;
+ n = res_nquerydomainN(statp, name, domain, &q);
+ return ((n < 0) ? n : q.n);
+}
+
+static int
+res_nquerydomainN(res_state statp, const char *name, const char *domain,
+ struct res_target *target)
+{
char nbuf[MAXDNAME];
const char *longname = nbuf;
int n, d;
@@ -408,7 +490,8 @@ res_nquerydomain(res_state statp,
#ifdef DEBUG
if (statp->options & RES_DEBUG)
printf(";; res_nquerydomain(%s, %s, %d, %d)\n",
- name, domain?domain:"<Nil>", class, type);
+ name, domain?domain:"<Nil>", target->qclass,
+ target->qtype);
#endif
if (domain == NULL) {
/*
@@ -433,9 +516,9 @@ res_nquerydomain(res_state statp,
RES_SET_H_ERRNO(statp, NO_RECOVERY);
return (-1);
}
- sprintf(nbuf, "%s.%s", name, domain);
+ snprintf(nbuf, sizeof(nbuf), "%s.%s", name, domain);
}
- return (res_nquery(statp, longname, class, type, answer, anslen));
+ return (res_nqueryN(statp, longname, target));
}
const char *
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081108151023.GA72747>