Re: [PATCH v2 06/12] gaih_inet: Split simple gethostbyname into its own function

[DJ Delorie <dj@redhat.com>]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> Add a free_at flag in gaih_result to indicate if res.at needs to be
> freed by the caller.

LGTM with one comment to be added.

Reviewed-by: DJ Delorie <dj@redhat.com>

> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index d7b6eae9fc..bcceab7d07 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -120,6 +120,7 @@ struct gaih_result
>  {
>    struct gaih_addrtuple *at;
>    char *canon;
> +  bool free_at;
>  };

Ok.

> @@ -565,6 +566,60 @@ out:
>    return result;
>  }
>  
> +/* If possible, call the simple, old functions, which do not support IPv6 scope
> +   ids, nor retrieving the canonical name.  */
> +
> +static int
> +try_simple_gethostbyname (const char *name, const struct addrinfo *req,
> +			  struct scratch_buffer *tmpbuf,
> +			  struct gaih_result *res)
> +{
> +  res->at = NULL;
> +
> +  if (req->ai_family != AF_INET || (req->ai_flags & AI_CANONNAME) != 0)
> +    return 0;
> +
> +  int rc;
> +  struct hostent th;
> +  struct hostent *h;
> +
> +  while (1)
> +    {
> +      rc = __gethostbyname2_r (name, AF_INET, &th, tmpbuf->data,
> +			       tmpbuf->length, &h, &h_errno);
> +      if (rc != ERANGE || h_errno != NETDB_INTERNAL)
> +	break;
> +      if (!scratch_buffer_grow (tmpbuf))
> +	return -EAI_MEMORY;
> +    }
> +
> +  if (rc == 0)
> +    {
> +      if (h != NULL)
> +	{
> +	  /* We found data, convert it.  */
> +	  if (!convert_hostent_to_gaih_addrtuple (req, AF_INET, h, &res->at))
> +	    return -EAI_MEMORY;
> +
> +	  res->free_at = true;


Please add a comment here that res->at will either be the result of a
realloc() or will be NULL, either of which may be safely passed to
free().  convert_hostent_to_gaih_addrtuple() has a true return path that
doesn't include the allocation, which may confuse future readers.

Otherwise ok

> +	  return 0;
> +	}
> +      if (h_errno == NO_DATA)
> +	return -EAI_NODATA;
> +
> +      return -EAI_NONAME;
> +    }
> +
> +  if (h_errno == NETDB_INTERNAL)
> +    return -EAI_SYSTEM;
> +  if (h_errno == TRY_AGAIN)
> +    return -EAI_AGAIN;
> +
> +  /* We made requests but they turned out no data.
> +     The name is known, though.  */
> +  return -EAI_NODATA;
> +}

Ok.

>  static int
>  gaih_inet (const char *name, const struct gaih_service *service,
>  	   const struct addrinfo *req, struct addrinfo **pai,
> @@ -610,6 +665,11 @@ gaih_inet (const char *name, const struct gaih_service *service,
>        else if (res.at != NULL)
>  	goto process_list;
>  
> +      if ((result = try_simple_gethostbyname (name, req, tmpbuf, &res)) != 0)
> +	goto free_and_return;
> +      else if (res.at != NULL)
> +	goto process_list;
> +

Ok.

> @@ -619,69 +679,6 @@ gaih_inet (const char *name, const struct gaih_service *service,
>        struct resolv_context *res_ctx = NULL;
>        bool do_merge = false;
>  
> -      /* If we do not have to look for IPv6 addresses or the canonical
> -	 name, use the simple, old functions, which do not support
> - . . .
> -
> -	  goto process_list;
> -	}

Ok.

> +  if (res.free_at)
> +    free (res.at);

Ok.