[PATCH v2 05/12] gaih_inet: make numeric lookup a separate routine

[DJ Delorie]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> . . .

git's default patch method results in too messy a patch to review in
this case, so I applied the patch and re-diffed it with alternate
options (diff -EZdbwpU5) and I'll review *that*.

LGTM.

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

> @@ -114,10 +114,16 @@ struct gaih_typeproto
> +struct gaih_result
> +{
> +  struct gaih_addrtuple *at;
> +  char *canon;
> +};

Ok.

> @@ -295,20 +301,20 @@ convert_hostent_to_gaih_addrtuple (const
> -      if (localcanon != NULL && canon == NULL)				      \
> +      if (localcanon != NULL && res.canon == NULL)			      \

Ok.

> -	  canon = canonbuf;						      \
> +	  res.canon = canonbuf;						      \

Ok.

>  static int
>  process_canonname (const struct addrinfo *req, const char *orig_name,
> -		   char **canonp)
> +		   struct gaih_result *res)
>  {
> -  char *canon = *canonp;
> +  char *canon = res->canon;

Ok.

>  
> -  *canonp = canon;
> +  res->canon = canon;

Ok.

> +/* Convert numeric addresses to binary into RES.  On failure, RES->AT is set to
> +   NULL and an error code is returned.  If AI_NUMERIC_HOST is not requested and
> +   the function cannot determine a result, RES->AT is set to NULL and 0
> +   returned.  */
> +
>  static int
> -gaih_inet (const char *name, const struct gaih_service *service,
> -	   const struct addrinfo *req, struct addrinfo **pai,
> -	   unsigned int *naddrs, struct scratch_buffer *tmpbuf)
> +text_to_binary_address (const char *name, const struct addrinfo *req,
> +			struct gaih_result *res)

Ok.

>  {
> -  struct gaih_servtuple st[sizeof (gaih_inet_typeproto)
> -			   / sizeof (struct gaih_typeproto)] = {0};
> -
> -  struct gaih_addrtuple *at = NULL;
> -  bool got_ipv6 = false;
> -  char *canon = NULL;
> -  const char *orig_name = name;
> -
> -  /* Reserve stack memory for the scratch buffer in the getaddrinfo
> -     function.  */
> -  size_t alloca_used = sizeof (struct scratch_buffer);
> -
> -  int rc;
> -  if ((rc = get_servtuples (service, req, st, tmpbuf)) != 0)
> -    return rc;
> -
> -  bool malloc_name = false;
> -  struct gaih_addrtuple *addrmem = NULL;
>    int result = 0;
>  
> -  if (name != NULL)
> -    {
> -      if (req->ai_flags & AI_IDN)
> -	{
> -	  char *out;
> -	  result = __idna_to_dns_encoding (name, &out);
> -	  if (result != 0)
> -	    return -result;
> -	  name = out;
> -	  malloc_name = true;
> -	}

"Moved", ok.

> +  struct gaih_addrtuple *at = res->at;
> +  assert (at != NULL);

Ok.

> -      uint32_t addr[4];
> -      if (__inet_aton_exact (name, (struct in_addr *) addr) != 0)
> +  memset (at->addr, 0, sizeof (at->addr));
> +  if (__inet_aton_exact (name, (struct in_addr *) at->addr) != 0)

Ok.

>  	{
> -	  at = alloca_account (sizeof (struct gaih_addrtuple), alloca_used);
> -	  at->scopeid = 0;
> -	  at->next = NULL;
> -

at is pre-filled, ok.

>  	  if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET)
> -	    {
> -	      memcpy (at->addr, addr, sizeof (at->addr));
>  	      at->family = AF_INET;
> -	    }

Ok.

>  	  else if (req->ai_family == AF_INET6 && (req->ai_flags & AI_V4MAPPED))
>  	    {
> -	      at->addr[3] = addr[0];
> +	  at->addr[3] = at->addr[0];
>  	      at->addr[2] = htonl (0xffff);
>  	      at->addr[1] = 0;
>  	      at->addr[0] = 0;

Ok.

> -	      goto free_and_return;
> +	  goto out;

Ok.

> -		  goto free_and_return;
> +	      goto out;
>  		}
> -	      canon = canonbuf;
> +	  res->canon = canonbuf;
>  	    }
> -
> -	  goto process_list;
> +      return 0;
>  	}

Ok.

>        char *scope_delim = strchr (name, SCOPE_DELIMITER);
>        int e;
>  
>        if (scope_delim == NULL)
> -	e = inet_pton (AF_INET6, name, addr);
> +    e = inet_pton (AF_INET6, name, at->addr);
>        else
> -	e = __inet_pton_length (AF_INET6, name, scope_delim - name, addr);
> +    e = __inet_pton_length (AF_INET6, name, scope_delim - name, at->addr);

Ok.

>        if (e > 0)
>  	{
> -	  at = alloca_account (sizeof (struct gaih_addrtuple),
> -			       alloca_used);
> -	  at->scopeid = 0;
> -	  at->next = NULL;
> -

Ok.

>  	  if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET6)
> -	    {
> -	      memcpy (at->addr, addr, sizeof (at->addr));
>  	      at->family = AF_INET6;
> -	    }

Ok.

>  	  else if (req->ai_family == AF_INET
> -		   && IN6_IS_ADDR_V4MAPPED (addr))
> +	       && IN6_IS_ADDR_V4MAPPED (at->addr))
>  	    {
> -	      at->addr[0] = addr[3];
> -	      at->addr[1] = addr[1];
> -	      at->addr[2] = addr[2];
> -	      at->addr[3] = addr[3];
> +	  at->addr[0] = at->addr[3];
>  	      at->family = AF_INET;

Ok.

>  	      result = -EAI_ADDRFAMILY;
> -	      goto free_and_return;
> +	  goto out;

Ok.

>  	  if (scope_delim != NULL
>  	      && __inet6_scopeid_pton ((struct in6_addr *) at->addr,
> -				       scope_delim + 1,
> -				       &at->scopeid) != 0)
> +				   scope_delim + 1, &at->scopeid) != 0)

Ok.

>  	    {
>  	      result = -EAI_NONAME;
> -	      goto free_and_return;
> +	  goto out;
>  	    }

Ok.

>  		  result = -EAI_MEMORY;
> -		  goto free_and_return;
> +	      goto out;

Ok.

> -	      canon = canonbuf;
> +	  res->canon = canonbuf;
> +	}
> +      return 0;

Ok.

> -	  goto process_list;
> +  if ((req->ai_flags & AI_NUMERICHOST))
> +    result = -EAI_NONAME;
> +
> +out:
> +  res->at = NULL;
> +  return result;
>  	}

Ok.

> -      if ((req->ai_flags & AI_NUMERICHOST) == 0)

Ok.

> +static int
> +gaih_inet (const char *name, const struct gaih_service *service,
> +	   const struct addrinfo *req, struct addrinfo **pai,
> +	   unsigned int *naddrs, struct scratch_buffer *tmpbuf)
> +{
> +  struct gaih_servtuple st[sizeof (gaih_inet_typeproto)
> +			   / sizeof (struct gaih_typeproto)] = {0};
> +
> +  bool got_ipv6 = false;
> +  const char *orig_name = name;
> +
> +  /* Reserve stack memory for the scratch buffer in the getaddrinfo
> +     function.  */
> +  size_t alloca_used = sizeof (struct scratch_buffer);
> +
> +  int rc;
> +  if ((rc = get_servtuples (service, req, st, tmpbuf)) != 0)
> +    return rc;
> +
> +  bool malloc_name = false;
> +  struct gaih_addrtuple *addrmem = NULL;
> +  int result = 0;
> +
> +  struct gaih_result res = {0};
> +  if (name != NULL)
>  	{
> +      if (req->ai_flags & AI_IDN)
> +	{
> +	  char *out;
> +	  result = __idna_to_dns_encoding (name, &out);
> +	  if (result != 0)
> +	    return -result;
> +	  name = out;
> +	  malloc_name = true;
> +	}
> +
> +      res.at = alloca_account (sizeof (struct gaih_addrtuple), alloca_used);
> +      res.at->scopeid = 0;
> +      res.at->next = NULL;
> +
> +      if ((result = text_to_binary_address (name, req, &res)) != 0)
> +	goto free_and_return;
> +      else if (res.at != NULL)
> +	goto process_list;
> +

"Moved", ok.

> -		      at = addrmem;
> +		  res.at = addrmem;

Ok.

> -		  struct gaih_addrtuple **pat = &at;
> +	      struct gaih_addrtuple **pat = &res.at;

Ok.

> -		      else if (canon == NULL)
> +		  else if (res.canon == NULL)

Ok.

> -			  canon = (*pat)->name = canonbuf;
> +		      res.canon = (*pat)->name = canonbuf;

Ok.

>  	      /* Always start afresh; continue should discard previous results
>  		 and the hosts database does not support merge.  */
> -	      at = NULL;
> -	      free (canon);
> +	  res.at = NULL;
> +	  free (res.canon);
>  	      free (addrmem);
> -	      canon = NULL;
> +	  res.canon = NULL;
>  	      addrmem = NULL;

Ok.

> -		      status = DL_CALL_FCT (fct4, (name, &at,
> +		  status = DL_CALL_FCT (fct4, (name, &res.at,

Ok.

> -		      at = NULL;
> +		  res.at = NULL;

Ok.

> -		      if ((req->ai_flags & AI_CANONNAME) != 0 && canon == NULL)
> +		  if ((req->ai_flags & AI_CANONNAME) != 0 && res.canon == NULL)
>  			{
> -			  char *canonbuf = __strdup (at->name);
> +		      char *canonbuf = __strdup (res.at->name);

Ok.

> -			  canon = canonbuf;
> +		      res.canon = canonbuf;
>  			}
>  
> -		      struct gaih_addrtuple **pat = &at;
> +		  struct gaih_addrtuple **pat = &res.at;

Ok.

> -		      struct gaih_addrtuple **pat = &at;
> +		  struct gaih_addrtuple **pat = &res.at;

Ok.

>  			  if ((req->ai_flags & AI_CANONNAME) != 0
> -			      && canon == NULL)
> +			  && res.canon == NULL)

Ok.

> -			      char *canonbuf = getcanonname (nip, at, name);
> +			  char *canonbuf = getcanonname (nip, res.at, name);

Ok.

> -			      canon = canonbuf;
> +			  res.canon = canonbuf;

Ok.

> -	}

Ok, matches "if (name != NULL) {"

>      process_list:
> -      if (at == NULL)
> +      if (res.at == NULL)

Ok.

>        struct gaih_addrtuple *atr;
> -      atr = at = alloca_account (sizeof (struct gaih_addrtuple), alloca_used);
> +      atr = res.at = alloca_account (sizeof (struct gaih_addrtuple),
> +				     alloca_used);
> -      memset (at, '\0', sizeof (struct gaih_addrtuple));
> +      memset (res.at, '\0', sizeof (struct gaih_addrtuple));

Ok.

>        if (req->ai_family == AF_UNSPEC)
>  	{
> -	  at->next = __alloca (sizeof (struct gaih_addrtuple));
> +	  res.at->next = __alloca (sizeof (struct gaih_addrtuple));
> -	  memset (at->next, '\0', sizeof (struct gaih_addrtuple));
> +	  memset (res.at->next, '\0', sizeof (struct gaih_addrtuple));
>  	}

Ok.

>        if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET6)
>  	{
> -	  at->family = AF_INET6;
> +	  res.at->family = AF_INET6;
>  	  if ((req->ai_flags & AI_PASSIVE) == 0)
> -	    memcpy (at->addr, &in6addr_loopback, sizeof (struct in6_addr));
> +	    memcpy (res.at->addr, &in6addr_loopback, sizeof (struct in6_addr));
> -	  atr = at->next;
> +	  atr = res.at->next;
>  	}

Ok.

>    {
>      /* Set up the canonical name if we need it.  */
> -    if ((result = process_canonname (req, orig_name, &canon)) != 0)
> +    if ((result = process_canonname (req, orig_name, &res)) != 0)
>        goto free_and_return;

Ok.

> -    struct gaih_addrtuple *at2 = at;
> +    struct gaih_addrtuple *at2 = res.at;

Ok.

>  	    /* We only add the canonical name once.  */
> -	    ai->ai_canonname = (char *) canon;
> +	    ai->ai_canonname = res.canon;
> -	    canon = NULL;
> +	    res.canon = NULL;

Ok.

>   free_and_return:
>    if (malloc_name)
>      free ((char *) name);
>    free (addrmem);
> -  free (canon);
> +  free (res.canon);

Ok.