Remove MULTI_PTRS_ARE_ALIASES to fix Wundef warning

Message ID 20140627173501.GA12370@spoyarek.pnq.redhat.com
State Superseded
Headers

Commit Message

Siddhesh Poyarekar June 27, 2014, 5:35 p.m. UTC
  The macro has been unconditionally defined as 1 in gethnamaddr.c and
not defined at all in dns-host.c.  As a result, the code excluded by
the macro has been dead for a while (since 1995 for gethnamaddr.c).
Removing the excluded bits do not cause any change to the generated
code on x86_64.

Siddhesh

	* resolv/gethnamaddr.c: Remove definition of
	MULTI_PTRS_ARE_ALIASES.
	(getanswer) [!MULTI_PTRS_ARE_ALIASES]: Remove code.
	* resolv/nss_dns/dns-host.c (getanswer_r)
	[MULTI_PTRS_ARE_ALIASES]: Likewise.

---
 resolv/gethnamaddr.c      | 18 ------------------
 resolv/nss_dns/dns-host.c | 22 ----------------------
 2 files changed, 40 deletions(-)
  

Comments

Roland McGrath June 27, 2014, 6:17 p.m. UTC | #1
The status quo would seem to suggest that those two files are out of synch.
It would be good for someone to grok what the code really does before we act.
  
Carlos O'Donell June 27, 2014, 6:20 p.m. UTC | #2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/27/2014 01:35 PM, Siddhesh Poyarekar wrote:
> The macro has been unconditionally defined as 1 in gethnamaddr.c and
> not defined at all in dns-host.c.  As a result, the code excluded by
> the macro has been dead for a while (since 1995 for gethnamaddr.c).
> Removing the excluded bits do not cause any change to the generated
> code on x86_64.

Looks OK.
 
> Siddhesh
> 
> 	* resolv/gethnamaddr.c: Remove definition of
> 	MULTI_PTRS_ARE_ALIASES.
> 	(getanswer) [!MULTI_PTRS_ARE_ALIASES]: Remove code.
> 	* resolv/nss_dns/dns-host.c (getanswer_r)
> 	[MULTI_PTRS_ARE_ALIASES]: Likewise.

OK, but there could be a bug here.

> ---
>  resolv/gethnamaddr.c      | 18 ------------------
>  resolv/nss_dns/dns-host.c | 22 ----------------------
>  2 files changed, 40 deletions(-)
> 
> diff --git a/resolv/gethnamaddr.c b/resolv/gethnamaddr.c
> index c73a0dc..9bbed59 100644
> --- a/resolv/gethnamaddr.c
> +++ b/resolv/gethnamaddr.c
> @@ -73,8 +73,6 @@ static char sccsid[] = "@(#)gethostnamadr.c	8.1 (Berkeley) 6/4/93";
>  # define LOG_AUTH 0
>  #endif
>  
> -#define MULTI_PTRS_ARE_ALIASES 1	/* XXX - experimental */
> -

OK.

>  #if defined(BSD) && (BSD >= 199103) && defined(AF_INET6)
>  # include <stdlib.h>
>  # include <string.h>
> @@ -359,7 +357,6 @@ getanswer (const querybuf *answer, int anslen, const char *qname, int qtype)
>  				had_error++;
>  				break;
>  			}
> -#if MULTI_PTRS_ARE_ALIASES
>  			cp += n;
>  			if (cp != erdata) {
>  				__set_h_errno (NO_RECOVERY);
> @@ -381,21 +378,6 @@ getanswer (const querybuf *answer, int anslen, const char *qname, int qtype)
>  				buflen -= n;
>  			}
>  			break;
> -#else
> -			host.h_name = bp;
> -			if (_res.options & RES_USE_INET6) {
> -				n = strlen(bp) + 1;	/* for the \0 */
> -				if (n >= MAXHOSTNAMELEN) {
> -					had_error++;
> -					break;
> -				}
> -				bp += n;
> -				buflen -= n;
> -				map_v4v6_hostent(&host, &bp, &buflen);
> -			}
> -			__set_h_errno (NETDB_SUCCESS);
> -			return (&host);
> -#endif

OK.

>  		case T_A:
>  		case T_AAAA:
>  			if (strcasecmp(host.h_name, bp) != 0) {
> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
> index a5f2c0a..258618e 100644
> --- a/resolv/nss_dns/dns-host.c
> +++ b/resolv/nss_dns/dns-host.c
> @@ -869,27 +869,6 @@ getanswer_r (const querybuf *answer, int anslen, const char *qname, int qtype,
>  	      ++had_error;
>  	      break;
>  	    }
> -#if MULTI_PTRS_ARE_ALIASES
> -	  cp += n;
> -	  if (haveanswer == 0)
> -	    result->h_name = bp;
> -	  else if (ap < &host_data->aliases[MAXALIASES-1])
> -	    *ap++ = bp;
> -	  else
> -	    n = -1;
> -	  if (n != -1)
> -	    {
> -	      n = strlen (bp) + 1;	/* for the \0 */
> -	      if (__builtin_expect (n, 0) >= MAXHOSTNAMELEN)
> -		{
> -		  ++had_error;
> -		  break;
> -		}
> -	      bp += n;
> -	      linebuflen -= n;
> -	    }
> -	  break;
> -#else

OK because it's been this way so long.

Do we have a bug here?

If the DNS server has multiple PTR records do we return them all?

I think we should since DNS doesn't make it invalid.

>  	  result->h_name = bp;
>  	  if (have_to_map)
>  	    {
> @@ -906,7 +885,6 @@ getanswer_r (const querybuf *answer, int anslen, const char *qname, int qtype,
>  	    }
>  	  *h_errnop = NETDB_SUCCESS;
>  	  return NSS_STATUS_SUCCESS;
> -#endif
>  	case T_A:
>  	case T_AAAA:
>  	  if (__builtin_expect (strcasecmp (result->h_name, bp), 0) != 0)
> 

Cheers,
Carlos.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTrbX9AAoJECXvCkNsKkr//QQIAMIx4AN4SoWCEdQ+oeubSFgT
7f8G9wq7EJaJX06aN4gAMvt7NGl2UUS56Clz2e+LjW8P6uaVJzcm2PiX/lVZh34Z
eahvioxKqvVlkpE1DENHco5vi/bua4i+MVUcFn82ZqKL6SgEgvqPUWo/ZIRSJLBe
l+0uDf3f6wHvpLgKzFKfULIiLBXZ+KgRZN2yptnKbDD/68URfOej5t0uvxt1WkGL
Qt1T+AifU/y6TKZbd8axftMYZmb9mIx68wihB4bwmnJzCPOtdSmIv12XQbdfRctK
2u4/U5XdcZ8hmhiIX54VYyxq2RV+Vmayd0UGXlyaiOTEwtQ7EW9krDakqUACpuw=
=Clsj
-----END PGP SIGNATURE-----
  
Siddhesh Poyarekar June 28, 2014, 1:27 a.m. UTC | #3
On Fri, Jun 27, 2014 at 02:20:45PM -0400, Carlos O'Donell wrote:
> OK because it's been this way so long.
> 

Not committing this yet since Roland also raised doubts about the
inconsistency.  Lets try to get that resolved first.

> Do we have a bug here?
> 
> If the DNS server has multiple PTR records do we return them all?
> 
> I think we should since DNS doesn't make it invalid.

It is inconsistent behaviour between getnameinfo and gethostbyaddr,
but I dont know if we ought to be fixing it now, which is why I went
with retaining old behaviour instead.

Why would multiple PTR records be useful for getnameinfo (or
gethostbyaddr for that matter)?  getnameinfo is capable of returning
only one name, so running through an entire list seems useless.  A
regular lookup won't expect a PTR record AFAICT.

So I would think that getnameinfo behaviour is not incorrect and
gethostbyaddr behaviour (it being a legacy function) be left alone.

Siddhesh
  
Roland McGrath June 28, 2014, 4:34 a.m. UTC | #4
I guess for now the inconsistency just makes me want to have the other code
forks around as documentation.  I suppose comments would do it just as
well.  Still my inclination is to make these something like:

/* Insert long new comment about the weirdness and referring to the other
   file where the opposite fork is used in equivalent code.  */
#if 0 /* was MULTI_PTRS_ARE_ALIASES */
  

Patch

diff --git a/resolv/gethnamaddr.c b/resolv/gethnamaddr.c
index c73a0dc..9bbed59 100644
--- a/resolv/gethnamaddr.c
+++ b/resolv/gethnamaddr.c
@@ -73,8 +73,6 @@  static char sccsid[] = "@(#)gethostnamadr.c	8.1 (Berkeley) 6/4/93";
 # define LOG_AUTH 0
 #endif
 
-#define MULTI_PTRS_ARE_ALIASES 1	/* XXX - experimental */
-
 #if defined(BSD) && (BSD >= 199103) && defined(AF_INET6)
 # include <stdlib.h>
 # include <string.h>
@@ -359,7 +357,6 @@  getanswer (const querybuf *answer, int anslen, const char *qname, int qtype)
 				had_error++;
 				break;
 			}
-#if MULTI_PTRS_ARE_ALIASES
 			cp += n;
 			if (cp != erdata) {
 				__set_h_errno (NO_RECOVERY);
@@ -381,21 +378,6 @@  getanswer (const querybuf *answer, int anslen, const char *qname, int qtype)
 				buflen -= n;
 			}
 			break;
-#else
-			host.h_name = bp;
-			if (_res.options & RES_USE_INET6) {
-				n = strlen(bp) + 1;	/* for the \0 */
-				if (n >= MAXHOSTNAMELEN) {
-					had_error++;
-					break;
-				}
-				bp += n;
-				buflen -= n;
-				map_v4v6_hostent(&host, &bp, &buflen);
-			}
-			__set_h_errno (NETDB_SUCCESS);
-			return (&host);
-#endif
 		case T_A:
 		case T_AAAA:
 			if (strcasecmp(host.h_name, bp) != 0) {
diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
index a5f2c0a..258618e 100644
--- a/resolv/nss_dns/dns-host.c
+++ b/resolv/nss_dns/dns-host.c
@@ -869,27 +869,6 @@  getanswer_r (const querybuf *answer, int anslen, const char *qname, int qtype,
 	      ++had_error;
 	      break;
 	    }
-#if MULTI_PTRS_ARE_ALIASES
-	  cp += n;
-	  if (haveanswer == 0)
-	    result->h_name = bp;
-	  else if (ap < &host_data->aliases[MAXALIASES-1])
-	    *ap++ = bp;
-	  else
-	    n = -1;
-	  if (n != -1)
-	    {
-	      n = strlen (bp) + 1;	/* for the \0 */
-	      if (__builtin_expect (n, 0) >= MAXHOSTNAMELEN)
-		{
-		  ++had_error;
-		  break;
-		}
-	      bp += n;
-	      linebuflen -= n;
-	    }
-	  break;
-#else
 	  result->h_name = bp;
 	  if (have_to_map)
 	    {
@@ -906,7 +885,6 @@  getanswer_r (const querybuf *answer, int anslen, const char *qname, int qtype,
 	    }
 	  *h_errnop = NETDB_SUCCESS;
 	  return NSS_STATUS_SUCCESS;
-#endif
 	case T_A:
 	case T_AAAA:
 	  if (__builtin_expect (strcasecmp (result->h_name, bp), 0) != 0)