From patchwork Fri Jun 30 18:05:53 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 21370 Received: (qmail 15688 invoked by alias); 30 Jun 2017 18:06:01 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 15663 invoked by uid 89); 30 Jun 2017 18:06:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E1A4D3D948 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=fweimer@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com E1A4D3D948 Subject: Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array From: Florian Weimer To: GNU C Library Cc: Andreas Schwab , Carlos O'Donell References: <8113b7c9-b9a8-7157-f957-53853dc4e232@redhat.com> Message-ID: <9d15b097-23d1-1aac-f037-7d3a5a21c538@redhat.com> Date: Fri, 30 Jun 2017 20:05:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <8113b7c9-b9a8-7157-f957-53853dc4e232@redhat.com> On 06/29/2017 08:35 PM, Florian Weimer wrote: > While working on the resolver code, I discovered some address copying > which appears to be unnecessary. The attached patch removes that. > > The comments refer to timing data, but we currently do not have that. > > Andreas, if I recall correctly, you wrote get_nsaddr. What do you think > about this change? This is the patch rebased to current master. Thanks, Florian resolv: Do not copy IPv4 addresses to IPv6 address array get_nsaddr already prefers IPv4 addresses if they are present (af_family != 0). If we ensure that nssocks is initialized to -1 and sockets are closed even without an allocated server address, there is no need to make these copies anymore. 2017-06-29 Florian Weimer * resolv/bits/types/res_state.h (struct __res_state): Rename _u._ext.nscount to _u._ext.__glibc_unused_nscount. * resolv/res_init.c (res_vinit): Adjust. Initialize nssocks member. * resolv/res_send.c (__libc_res_nsend): Do not copy IPv4 nameserver addresses. * resolv/res-close.c (__res_iclose): Close sockets even if the name server address has not been allocated. * support/resolv_test.c (resolv_test_start): Do not clear _res._u._ext.nscount because it is unused. diff --git a/resolv/bits/types/res_state.h b/resolv/bits/types/res_state.h index cee4b6d..d274cf3 100644 --- a/resolv/bits/types/res_state.h +++ b/resolv/bits/types/res_state.h @@ -40,7 +40,7 @@ struct __res_state { union { char pad[52]; /* On an i386 this means 512b total. */ struct { - uint16_t nscount; + uint16_t __glibc_unused_nscount; uint16_t nsmap[MAXNS]; int nssocks[MAXNS]; uint16_t nscount6; diff --git a/resolv/res-close.c b/resolv/res-close.c index 73f18d1..3195298 100644 --- a/resolv/res-close.c +++ b/resolv/res-close.c @@ -97,19 +97,18 @@ __res_iclose (res_state statp, bool free_addr) statp->_flags &= ~(RES_F_VC | RES_F_CONN); } for (int ns = 0; ns < statp->nscount; ns++) - if (statp->_u._ext.nsaddrs[ns] != NULL) - { - if (statp->_u._ext.nssocks[ns] != -1) - { - close_not_cancel_no_status (statp->_u._ext.nssocks[ns]); - statp->_u._ext.nssocks[ns] = -1; - } - if (free_addr) - { - free (statp->_u._ext.nsaddrs[ns]); - statp->_u._ext.nsaddrs[ns] = NULL; - } - } + { + if (statp->_u._ext.nssocks[ns] != -1) + { + close_not_cancel_no_status (statp->_u._ext.nssocks[ns]); + statp->_u._ext.nssocks[ns] = -1; + } + if (free_addr) + { + free (statp->_u._ext.nsaddrs[ns]); + statp->_u._ext.nsaddrs[ns] = NULL; + } + } } libc_hidden_def (__res_iclose) diff --git a/resolv/res_init.c b/resolv/res_init.c index 5d8b2c9..8f7b8dc 100644 --- a/resolv/res_init.c +++ b/resolv/res_init.c @@ -155,9 +155,12 @@ res_vinit_1 (res_state statp, bool preinit, FILE *fp, char **buffer) statp->_flags = 0; statp->__glibc_unused_qhook = NULL; statp->__glibc_unused_rhook = NULL; - statp->_u._ext.nscount = 0; + statp->_u._ext.__glibc_unused_nscount = 0; for (int n = 0; n < MAXNS; n++) - statp->_u._ext.nsaddrs[n] = NULL; + { + statp->_u._ext.nsaddrs[n] = NULL; + statp->_u._ext.nssocks[n] = -1; + } /* Allow user to override the local domain definition. */ if ((cp = getenv ("LOCALDOMAIN")) != NULL) @@ -328,7 +331,6 @@ res_vinit_1 (res_state statp, bool preinit, FILE *fp, char **buffer) statp->nsaddr_list[nserv].sin_family = 0; statp->_u._ext.nsaddrs[nserv] = sa6; - statp->_u._ext.nssocks[nserv] = -1; have_serv6 = true; nserv++; } diff --git a/resolv/res_send.c b/resolv/res_send.c index a7daae8..6ec957f 100644 --- a/resolv/res_send.c +++ b/resolv/res_send.c @@ -423,54 +423,6 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen, gotsomewhere = 0; terrno = ETIMEDOUT; - /* - * If the ns_addr_list in the resolver context has changed, then - * invalidate our cached copy and the associated timing data. - */ - if (EXT(statp).nscount != 0) { - int needclose = 0; - - if (EXT(statp).nscount != statp->nscount) - needclose++; - else - for (unsigned int ns = 0; ns < statp->nscount; ns++) { - if (statp->nsaddr_list[ns].sin_family != 0 - && !sock_eq((struct sockaddr_in6 *) - &statp->nsaddr_list[ns], - EXT(statp).nsaddrs[ns])) - { - needclose++; - break; - } - } - if (needclose) { - __res_iclose(statp, false); - EXT(statp).nscount = 0; - } - } - - /* - * Maybe initialize our private copy of the ns_addr_list. - */ - if (EXT(statp).nscount == 0) { - for (unsigned int ns = 0; ns < statp->nscount; ns++) { - EXT(statp).nssocks[ns] = -1; - if (statp->nsaddr_list[ns].sin_family == 0) - continue; - if (EXT(statp).nsaddrs[ns] == NULL) - EXT(statp).nsaddrs[ns] = - malloc(sizeof (struct sockaddr_in6)); - if (EXT(statp).nsaddrs[ns] != NULL) - memset (mempcpy(EXT(statp).nsaddrs[ns], - &statp->nsaddr_list[ns], - sizeof (struct sockaddr_in)), - '\0', - sizeof (struct sockaddr_in6) - - sizeof (struct sockaddr_in)); - } - EXT(statp).nscount = statp->nscount; - } - /* Name server index offset. Used to implement RES_ROTATE. */ unsigned int ns_offset = nameserver_offset (statp); diff --git a/support/resolv_test.c b/support/resolv_test.c index 050cd71..b5df5ff 100644 --- a/support/resolv_test.c +++ b/support/resolv_test.c @@ -1103,7 +1103,6 @@ resolv_test_start (struct resolv_redirect_config config) /* Disable IPv6 name server addresses. The code below only overrides the IPv4 addresses. */ __res_iclose (&_res, true); - _res._u._ext.nscount = 0; /* Redirect queries to the server socket. */ if (test_verbose)