Re: [PATCH v2 04/12] gaih_inet: Simplify service resolution

[DJ Delorie <dj@redhat.com>]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> Refactor the code to split out the service resolution code into a
> separate function.  Allocate the service tuples array just once to the
> size of the typeproto array, thus avoiding the unnecessary pointer
> chasing and stack allocations.

LGTM

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

>  struct gaih_servtuple
>    {
> -    struct gaih_servtuple *next;
>      int socktype;
>      int protocol;
>      int port;
> +    bool set;
>    };
>  
> -static const struct gaih_servtuple nullserv;
> -

Ok.

> @@ -180,11 +178,11 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
>      }
>    while (r);
>  
> -  st->next = NULL;
>    st->socktype = tp->socktype;
>    st->protocol = ((tp->protoflag & GAI_PROTO_PROTOANY)
>  		  ? req->ai_protocol : tp->protocol);
>    st->port = s->s_port;
> +  st->set = true;

Ok.

> @@ -375,20 +373,11 @@ process_canonname (const struct addrinfo *req, const char *orig_name,
>  }
>  
>  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)
> +get_servtuples (const struct gaih_service *service, const struct addrinfo *req,
> +		struct gaih_servtuple *st, struct scratch_buffer *tmpbuf)
>  {

Split out but the names of passed variables remain the same; ok.

> +  int i;
>    const struct gaih_typeproto *tp = gaih_inet_typeproto;
> -  struct gaih_servtuple *st = (struct gaih_servtuple *) &nullserv;
> -  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);

This part done in the parent function; ok.

> @@ -410,98 +399,88 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  	}
>      }
>  
> -  int port = 0;
> -  if (service != NULL)
> +  if (service != NULL && (tp->protoflag & GAI_PROTO_NOSERVICE) != 0)
> +    return -EAI_SERVICE;
> +
> +  if (service == NULL || service->num >= 0)
>      {
> -      if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0)
> -	return -EAI_SERVICE;
> +      int port = service != NULL ? htons (service->num) : 0;

Ok.

> +      if (req->ai_socktype || req->ai_protocol)
>  	{
> +	  st[0].socktype = tp->socktype;
> +	  st[0].protocol = ((tp->protoflag & GAI_PROTO_PROTOANY)
> +			  ? req->ai_protocol : tp->protocol);
> +	  st[0].port = port;
> +	  st[0].set = true;
> +	  return 0;
> +	}

ok.


> +      /* Neither socket type nor protocol is set.  Return all socket types
> +	 we know about.  */
> +      for (i = 0, ++tp; tp->name[0]; ++tp)
> +	if (tp->defaultflag)
> +	  {
> +	    st[i].socktype = tp->socktype;
> +	    st[i].protocol = tp->protocol;
> +	    st[i].port = port;
> +	    st[i++].set = true;
> +	  }

Ok.

> -      if (service->num < 0)
> -	  if (tp->name[0])
> -	    {
> -	      st = (struct gaih_servtuple *)
> -		alloca_account (sizeof (struct gaih_servtuple), alloca_used);
> -
> -	      int rc = gaih_inet_serv (service->name, tp, req, st, tmpbuf);
> -	      if (__glibc_unlikely (rc != 0))
> -		return rc;
> -	    }
> -	  else
> -	    {
> -	      struct gaih_servtuple **pst = &st;
> -	      for (tp++; tp->name[0]; tp++)
> -		{
> -		  struct gaih_servtuple *newp;
>  
> -		  if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0)
> -		    continue;
>  
> -		  if (req->ai_socktype != 0
> -		      && req->ai_socktype != tp->socktype)
> -		    continue;
> -		  if (req->ai_protocol != 0
> -		      && !(tp->protoflag & GAI_PROTO_PROTOANY)
> -		      && req->ai_protocol != tp->protocol)
> -		    continue;
>  
> -		  newp = (struct gaih_servtuple *)
> -		    alloca_account (sizeof (struct gaih_servtuple),
> -				    alloca_used);
> +      return 0;
> +    }

Ok.

> -		  if (gaih_inet_serv (service->name,
> -				      tp, req, newp, tmpbuf) != 0)
> -		    continue;

Ok.

> +  if (tp->name[0])
> +    return gaih_inet_serv (service->name, tp, req, st, tmpbuf);

Ok.

> +  for (i = 0, tp++; tp->name[0]; tp++)
> +      if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0)
> +	continue;
> +      if (req->ai_socktype != 0
> +	  && req->ai_socktype != tp->socktype)
> +	continue;
> +      if (req->ai_protocol != 0
> +	  && !(tp->protoflag & GAI_PROTO_PROTOANY)
> +	  && req->ai_protocol != tp->protocol)
> +	continue;
> +      if (gaih_inet_serv (service->name,
> +			  tp, req, &st[i], tmpbuf) != 0)
> +	continue;
> +      i++;

> +  if (!st[0].set)
> +    return -EAI_SERVICE;
> +
> +  return 0;
> +}

Ok.

> -		  *pst = newp;
> -		  pst = &(newp->next);
> -		}
> -	      if (st == (struct gaih_servtuple *) &nullserv)
> -		return -EAI_SERVICE;
> -	    }
> -	}
> -      else
> -	{
> -	  port = htons (service->num);
> -	  goto got_port;
> -	}
> -    }
> -  else
>      {
> -    got_port:
>  
> -      if (req->ai_socktype || req->ai_protocol)
> -	{
> -	  st = alloca_account (sizeof (struct gaih_servtuple), alloca_used);
> -	  st->next = NULL;
> -	  st->socktype = tp->socktype;
> -	  st->protocol = ((tp->protoflag & GAI_PROTO_PROTOANY)
> -			  ? req->ai_protocol : tp->protocol);
> -	  st->port = port;
> -	}
> -      else
> -	{
> -	  /* Neither socket type nor protocol is set.  Return all socket types
> -	     we know about.  */
> -	  struct gaih_servtuple **lastp = &st;
> -	  for (++tp; tp->name[0]; ++tp)
> -	    if (tp->defaultflag)
> -	      {
> -		struct gaih_servtuple *newp;
>  
> -		newp = alloca_account (sizeof (struct gaih_servtuple),
> -				       alloca_used);
> -		newp->next = NULL;
> -		newp->socktype = tp->socktype;
> -		newp->protocol = tp->protocol;
> -		newp->port = port;
>  
> -		*lastp = newp;
> -		lastp = &newp->next;
> -	      }
> -	}
>      }

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};
> +
> +  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;
> +

Ok.

> -    struct gaih_servtuple *st2;

Ok.

> -	for (st2 = st; st2 != NULL; st2 = st2->next)
> +	for (int i = 0; st[i].set; i++)

Ok.
> -	    ai->ai_socktype = st2->socktype;
> -	    ai->ai_protocol = st2->protocol;
> +	    ai->ai_socktype = st[i].socktype;
> +	    ai->ai_protocol = st[i].protocol;

Ok.

> -		sin6p->sin6_port = st2->port;
> +		sin6p->sin6_port = st[i].port;

Ok.

> -		sinp->sin_port = st2->port;
> +		sinp->sin_port = st[i].port;

Ok.