Re: [PATCH][BZ #14719] Return EAI_SYSTEM from getaddrinfo if we run out of fds

[Andreas Jaeger <aj at suse dot com>]

On Tuesday, October 23, 2012 17:18:35 Siddhesh Poyarekar wrote:
> Hi,
> 
> As described in the bug report, the current behaviour for getaddrinfo
> when it runs out of file descriptors is to return EAI_NOTFOUND, which
> is incorrect.  Since a system error has occurred, it should be
> returning EAI_SYSTEM so that the real cause of the failure could be
> seen from errno.  Attached patch does that.  Tested on x86_64 Fedora
> 16 for AF_INET, AF_UNSPEC and AF_INET6.  OK to commit?
> 
> Regards,
> Siddhesh
> 
> ChangeLog:
> 
> 	[BZ #14719]
> 	* nss/getXXbyYY_r.c (INTERNAL (REENTRANT_NAME)): Set h_errno to
> 	NETDB_INTERNAL when NSS_STATUS_UNAVAIL.
> 	* resolv/nss_dns/dns-host.c (_nss_dns_gethostbyname3_r): Set
> 	h_errno to NETDB_INTERNAL when errno is EMFILE or ENFILE.
> 	(_nss_dns_gethostbyname4_r): Likewise.
> 	* sysdeps/posix/getaddrinfo.c (gaih_inet): Set result to
> 	EAI_SYSTEM if NSS_STATUS_UNAVAIL.

I have a few questions and comments - and would like somebody else 
with more knowledge of this code to review it.

> diff --git a/nss/getXXbyYY_r.c b/nss/getXXbyYY_r.c
> index f296ed1..dfa0dfc 100644
> --- a/nss/getXXbyYY_r.c
> +++ b/nss/getXXbyYY_r.c
> 
> @@ -284,8 +284,13 @@ done:
>  #endif
>  
>    *result = status == NSS_STATUS_SUCCESS ? resbuf : NULL;
>  
>  #ifdef NEED_H_ERRNO
> 
> -  if (status != NSS_STATUS_SUCCESS && ! any_service)
> -    /* We were not able to use any service.  */
> +  if (status == NSS_STATUS_UNAVAIL)
> +    /* Either we failed to lookup the functions or the functions
> themselves had a +       system error.  Set NETDB_INTERNAL here to
> let the caller know that the +       errno may have the real reason
> for failure.  */

This line is to long, please wrap at 78 characters.

> +      *h_errnop = NETDB_INTERNAL;
> +  else if (status != NSS_STATUS_SUCCESS && !any_service)
> +    /* We were no able to use any service.  */
> 
>      *h_errnop = NO_RECOVERY;
>  
>  #endif
>  #ifdef POSTPROCESS
> 
> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
> index 6b62c05..102afb2 100644
> --- a/resolv/nss_dns/dns-host.c
> +++ b/resolv/nss_dns/dns-host.c
> @@ -199,6 +199,10 @@ _nss_dns_gethostbyname3_r (const char *name, int
> af, struct hostent *result,> 
>  	  status = NSS_STATUS_TRYAGAIN;
>  	  h_errno = TRY_AGAIN;
>  	  break;
> 
> +	/* System has run out of file descriptors.  */
> +	case EMFILE:
> +	case ENFILE:
> +	  h_errno = NETDB_INTERNAL;

No break? In that case, please document that this is intented, e.g. say "Fall through"
> 
>  	case ECONNREFUSED:
>  	
>  	case ETIMEDOUT:
>  	  status = NSS_STATUS_UNAVAIL;


> @@ -311,14 +315,25 @@ _nss_dns_gethostbyname4_r (const char *name,
> struct gaih_addrtuple **pat,> 
>  			      &ans2p, &nans2p, &resplen2);
>    
>    if (n < 0)
>    
>      {
> 
> -      if (errno == ESRCH)
> +      switch (errno)
> 
>  	{
> 
> +	case ESRCH:
>  	  status = NSS_STATUS_TRYAGAIN;
>  	  h_errno = TRY_AGAIN;
> 
> +	  break;
> +	/* System has run out of file descriptors.  */
> +	case EMFILE:
> +	case ENFILE:
> +	  h_errno = NETDB_INTERNAL;

Again, a fall through that should get documented.

> +	case ECONNREFUSED:
> +	case ETIMEDOUT:
> +	  status = NSS_STATUS_UNAVAIL;
> +	  break;
> +	default:
> +	  status = NSS_STATUS_NOTFOUND;
> +	  break;

You're setting it to UNAVAIL also with ETIMEDOUT which wasn't done before. 
Is that correct? It seems to mirror the conditions above

> 
>  	}
> 
> -      else
> -	status = (errno == ECONNREFUSED
> -		  ? NSS_STATUS_UNAVAIL : NSS_STATUS_NOTFOUND);
> +
> 
>        *herrnop = h_errno;
>        if (h_errno == TRY_AGAIN)
>  	
>  	*errnop = EAGAIN;
> 
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index 672571e..ad5ae01 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -1049,6 +1049,12 @@ gaih_inet (const char *name, const struct
> gaih_service *service,> 
>  	  _res.options |= old_res_options & RES_USE_INET6;
> 
> +	  if (status == NSS_STATUS_UNAVAIL)
> +	    {
> +	      result = GAIH_OKIFUNSPEC | -EAI_SYSTEM;
> +	      goto free_and_return;
> +	    }
> +
> 
>  	  if (no_data != 0 && no_inet6_data != 0)
>  	  
>  	    {
>  	    
>  	      /* If both requests timed out report this.  */

Andreas
-- 
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126