Re: [PATCH v2 08/12] gaih_inet: separate nss lookup loop into its own function

[DJ Delorie <dj@redhat.com>]

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

One memory leak needs fixing...

> +static void
> +gaih_result_reset (struct gaih_result *res)
> +{
> +  if (res->free_at)
> +    free (res->at);
> +  free (res->canon);
> +  memset (res, 0, sizeof (*res));
> +}

(I think this fixes the problem in [07/12] ;)

> -/* Convert struct hostent to a list of struct gaih_addrtuple objects.
> -   h_name is not copied, and the struct hostent object must not be
> -   deallocated prematurely.  *RESULT must be NULL or a pointer to a
> -   linked-list.  The new addresses are appended at the end.  */
> +/* Convert struct hostent to a list of struct gaih_addrtuple objects.  h_name
> +   is not copied, and the struct hostent object must not be deallocated
> +   prematurely.  *RESULT must be NULL or a pointer to a linked-list.  The new
> +   addresses are appended at the end.  */

no changes, ok.

>  static bool
> -convert_hostent_to_gaih_addrtuple (const struct addrinfo *req,
> -				   int family,
> -				   struct hostent *h,
> -				   struct gaih_addrtuple **result)
> +convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, int family,
> +				   struct hostent *h, struct gaih_result *res)

Ok.

> -  struct gaih_addrtuple *array = *result;
> +  struct gaih_addrtuple *array = res->at;

Ok.

> -  array = realloc (*result, (old + count) * sizeof (*array));
> +  array = res->at = realloc (res->at, (old + count) * sizeof (*array));
>  
>    if (array == NULL)
>      return false;
>  
> -  *result = array;

This is not the same - if realloc fails, the original res->at is lost
and can no longer be free'd.

> +  res->got_ipv6 = family == AF_INET6;
> +  res->free_at = true;

Ok.

> -	  goto free_and_return;						      \
> +	  goto out;							      \

Ok.

> -	  goto free_and_return;						      \
> +	  goto out;							      \

Ok.

> -      if (!convert_hostent_to_gaih_addrtuple (req, _family, &th, &addrmem))   \
> +      if (!convert_hostent_to_gaih_addrtuple (req, _family, &th, res))	      \

Ok.

> -	  goto free_and_return;						      \
> +	  goto out;							      \

Ok.

> -      *pat = addrmem;							      \
>  									      \
> -      if (localcanon != NULL && res.canon == NULL)			      \
> +      if (localcanon != NULL && res->canon == NULL)			      \

Ok.

> -	      goto free_and_return;					      \
> +	      goto out;							      \

Ok.

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

Ok.

> -      if (_family == AF_INET6 && *pat != NULL)				      \
> -	res.got_ipv6 = true;						      \

Ok.

> +static int
> +get_nss_addresses (const char *name, const struct addrinfo *req,
> +		   struct scratch_buffer *tmpbuf, struct gaih_result *res)
> +{
> + . . .
> +}

Moved, ok.

> -	  if (!convert_hostent_to_gaih_addrtuple (req, AF_INET, h, &res->at))
> +	  if (!convert_hostent_to_gaih_addrtuple (req, AF_INET, h, res))
>  	    return -EAI_MEMORY;

Ok.

> -      int no_data = 0;
> -      int no_inet6_data = 0;
> - . . . 
> -	}

Moved, ok.

> +      if ((result = get_nss_addresses (name, req, tmpbuf, &res)) != 0)
> +	goto free_and_return;
> +      else if (res.at != NULL)
> +	goto process_list;

Ok.

> -    process_list:
> -      if (res.at == NULL)
> -	{
> -	  result = -EAI_NONAME;
> -	  goto free_and_return;
> -	}
> +      /* None of the lookups worked, so name not found.  */
> +      result = -EAI_NONAME;
> +      goto free_and_return;

Ok.

>  
> +process_list:
>    {
>      /* Set up the canonical name if we need it.  */
>      if ((result = process_canonname (req, orig_name, &res)) != 0)

Ok.