diff mbox

[4/4] resolv: fix rotate option

Message ID 1402673533-13243-5-git-send-email-aurelien@aurel32.net
State Committed
Headers

Commit Message

Aurelien Jarno June 13, 2014, 3:32 p.m. UTC 
  The rotate option doesn't work correctly, and only send the query to the
same server (the second in the list). The rotation code in itself is not
broken, but the nsaddrs structure is reinitialized each time at the
beginning of __libc_res_nsend unless RES_STAYOPEN is enabled.

This is due to a call to __res_iclose from the end of __libc_res_nsend
when answers from the name server have been received. This function
closes all the sockets, but doesn't free the addresses (it can do that,
but in that case the second argument is false).

This patch change the code of __res_iclose to clear
statp->_u._ext.nsinit only when the addresses are actually freed.

2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>

	* resolv/res_init.c (__res_iclose): Only clear nsinit if the
	addresses have been freed.

Comments

Siddhesh Poyarekar June 20, 2014, 3:04 a.m. UTC | #1 
On Fri, Jun 13, 2014 at 05:32:13PM +0200, Aurelien Jarno wrote:
> The rotate option doesn't work correctly, and only send the query to the
> same server (the second in the list). The rotation code in itself is not
> broken, but the nsaddrs structure is reinitialized each time at the
> beginning of __libc_res_nsend unless RES_STAYOPEN is enabled.
> 
> This is due to a call to __res_iclose from the end of __libc_res_nsend
> when answers from the name server have been received. This function
> closes all the sockets, but doesn't free the addresses (it can do that,
> but in that case the second argument is false).
> 
> This patch change the code of __res_iclose to clear
> statp->_u._ext.nsinit only when the addresses are actually freed.
> 
> 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
> 
> 	* resolv/res_init.c (__res_iclose): Only clear nsinit if the
> 	addresses have been freed.

Looks good to me, but I think this deserves a separate bug report from
the general 'ipv6 name resolution is broken' bug report you filed for
this patchset.

Siddhesh

>  
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> index 95564af..42e16b6 100644
> --- a/resolv/res_init.c
> +++ b/resolv/res_init.c
> @@ -621,7 +621,8 @@ __res_iclose(res_state statp, bool free_addr) {
>  				statp->_u._ext.nsaddrs[ns] = NULL;
>  			}
>  		}
> -	statp->_u._ext.nsinit = 0;
> +	if (free_addr)
> +		statp->_u._ext.nsinit = 0;
>  }
>  libc_hidden_def (__res_iclose)
>  
> -- 
> 2.0.0
>
H.J. Lu Jan. 6, 2015, 5:02 p.m. UTC | #2 
On Thu, Jun 19, 2014 at 8:04 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Fri, Jun 13, 2014 at 05:32:13PM +0200, Aurelien Jarno wrote:
>> The rotate option doesn't work correctly, and only send the query to the
>> same server (the second in the list). The rotation code in itself is not
>> broken, but the nsaddrs structure is reinitialized each time at the
>> beginning of __libc_res_nsend unless RES_STAYOPEN is enabled.
>>
>> This is due to a call to __res_iclose from the end of __libc_res_nsend
>> when answers from the name server have been received. This function
>> closes all the sockets, but doesn't free the addresses (it can do that,
>> but in that case the second argument is false).
>>
>> This patch change the code of __res_iclose to clear
>> statp->_u._ext.nsinit only when the addresses are actually freed.
>>
>> 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
>>
>>       * resolv/res_init.c (__res_iclose): Only clear nsinit if the
>>       addresses have been freed.
>
> Looks good to me, but I think this deserves a separate bug report from
> the general 'ipv6 name resolution is broken' bug report you filed for
> this patchset.
>

I checked it in.

Thanks.
Joseph Myers Jan. 6, 2015, 5:23 p.m. UTC | #3 
On Tue, 6 Jan 2015, H.J. Lu wrote:

> On Thu, Jun 19, 2014 at 8:04 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> > On Fri, Jun 13, 2014 at 05:32:13PM +0200, Aurelien Jarno wrote:

> >> 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
> >>
> >>       * resolv/res_init.c (__res_iclose): Only clear nsinit if the
> >>       addresses have been freed.
> >
> > Looks good to me, but I think this deserves a separate bug report from
> > the general 'ipv6 name resolution is broken' bug report you filed for
> > this patchset.
> >
> 
> I checked it in.

Where are the separate bug report requested, BZ#N notation in the 
ChangeLog entry and listing of that bug in the list of fixed bugs in NEWS, 
as per Siddhesh's review?
Andreas Schwab Feb. 18, 2015, 4:54 p.m. UTC | #4 
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> On Fri, Jun 13, 2014 at 05:32:13PM +0200, Aurelien Jarno wrote:
>> The rotate option doesn't work correctly, and only send the query to the
>> same server (the second in the list). The rotation code in itself is not
>> broken, but the nsaddrs structure is reinitialized each time at the
>> beginning of __libc_res_nsend unless RES_STAYOPEN is enabled.
>> 
>> This is due to a call to __res_iclose from the end of __libc_res_nsend
>> when answers from the name server have been received. This function
>> closes all the sockets, but doesn't free the addresses (it can do that,
>> but in that case the second argument is false).
>> 
>> This patch change the code of __res_iclose to clear
>> statp->_u._ext.nsinit only when the addresses are actually freed.
>> 
>> 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
>> 
>> 	* resolv/res_init.c (__res_iclose): Only clear nsinit if the
>> 	addresses have been freed.
>
> Looks good to me

This is broken.  It means that __libc_res_nsend no longer reinitializes
its cached copy of the name servers.

Andreas.
diff mbox

Patch

diff --git a/resolv/res_init.c b/resolv/res_init.c
index 95564af..42e16b6 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -621,7 +621,8 @@  __res_iclose(res_state statp, bool free_addr) {
 				statp->_u._ext.nsaddrs[ns] = NULL;
 			}
 		}
-	statp->_u._ext.nsinit = 0;
+	if (free_addr)
+		statp->_u._ext.nsinit = 0;
 }
 libc_hidden_def (__res_iclose)