From owner-freebsd-net@FreeBSD.ORG Sat Nov 8 15:05:47 2008 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4357810656A8 for ; Sat, 8 Nov 2008 15:05:47 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.9.129]) by mx1.freebsd.org (Postfix) with ESMTP id AA4C48FC16 for ; Sat, 8 Nov 2008 15:05:46 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id 9DD09730AC; Sat, 8 Nov 2008 16:10:23 +0100 (CET) Date: Sat, 8 Nov 2008 16:10:23 +0100 From: Luigi Rizzo To: Hajimu UMEMOTO Message-ID: <20081108151023.GA72747@onelab2.iet.unipi.it> References: <20081106192103.GA46651@onelab2.iet.unipi.it> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="x+6KMIRAuhnl3hBn" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Cc: net@freebsd.org Subject: Re: Two copies of resolver routines in libc ? X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 08 Nov 2008 15:05:47 -0000 --x+6KMIRAuhnl3hBn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 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 --x+6KMIRAuhnl3hBn Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="resolv-ubwp.diff" 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 #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:"", class, type); + name, domain?domain:"", 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 * --x+6KMIRAuhnl3hBn--