[PATCH 0/3] More getaddrinfo refactoring

[PATCH 0/3] More getaddrinfo refactoring

[Siddhesh Poyarekar]

These are refactoring patches in parts of getaddrinfo implementation other than
gaih_inet, aimed at improving readability of the code.

Tested with x86_64 and i686 build and check.

Siddhesh Poyarekar (3):
  gaiconf_init: Refactor some bits for readability
  gai_init: Avoid jumping from if condition to its else counterpart
  getaddrinfo: Refactor code for readability

 sysdeps/posix/getaddrinfo.c | 651 +++++++++++++++++++-----------------
 1 file changed, 345 insertions(+), 306 deletions(-)

-- 
2.35.1

[PATCH 1/3] gaiconf_init: Refactor some bits for readability

[Siddhesh Poyarekar]

Split out line processing for `label`, `precedence` and `scopev4` into
separate functions instead of the gotos.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 sysdeps/posix/getaddrinfo.c | 149 ++++++++++++++++++++----------------
 1 file changed, 84 insertions(+), 65 deletions(-)

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 18dccd5924..95a61d7238 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -1761,6 +1761,66 @@ scopecmp (const void *p1, const void *p2)
   return 1;
 }
 
+static bool
+add_prefixlist (struct prefixlist **listp, size_t *lenp, bool *nullbitsp,
+		char *val1, char *val2, char **pos)
+{
+  struct in6_addr prefix;
+  unsigned long int bits;
+  unsigned long int val;
+  char *endp;
+
+  bits = 128;
+  __set_errno (0);
+  char *cp = strchr (val1, '/');
+  if (cp != NULL)
+    *cp++ = '\0';
+  *pos = cp;
+  if (inet_pton (AF_INET6, val1, &prefix)
+      && (cp == NULL
+	  || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
+	  || errno != ERANGE)
+      && *endp == '\0'
+      && bits <= 128
+      && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
+	  || errno != ERANGE)
+      && *endp == '\0'
+      && val <= INT_MAX)
+    {
+      struct prefixlist *newp = malloc (sizeof (*newp));
+      if (newp == NULL)
+	return false;
+
+      memcpy (&newp->entry.prefix, &prefix, sizeof (prefix));
+      newp->entry.bits = bits;
+      newp->entry.val = val;
+      newp->next = *listp;
+      *listp = newp;
+      ++*lenp;
+      *nullbitsp |= bits == 0;
+    }
+  return true;
+}
+
+static bool
+add_scopelist (struct scopelist **listp, size_t *lenp, bool *nullbitsp,
+	       const struct in6_addr *prefixp, unsigned long int bits,
+	       unsigned long int val)
+{
+  struct scopelist *newp = malloc (sizeof (*newp));
+  if (newp == NULL)
+    return false;
+
+  newp->entry.netmask = htonl (bits != 96 ? (0xffffffff << (128 - bits)) : 0);
+  newp->entry.addr32 = (prefixp->s6_addr32[3] & newp->entry.netmask);
+  newp->entry.scope = val;
+  newp->next = *listp;
+  *listp = newp;
+  ++*lenp;
+  *nullbitsp |= bits == 96;
+
+  return true;
+}
 
 static void
 gaiconf_init (void)
@@ -1836,55 +1896,17 @@ gaiconf_init (void)
 	  /*  Ignore the rest of the line.  */
 	  *cp = '\0';
 
-	  struct prefixlist **listp;
-	  size_t *lenp;
-	  bool *nullbitsp;
 	  switch (cmdlen)
 	    {
 	    case 5:
 	      if (strcmp (cmd, "label") == 0)
 		{
-		  struct in6_addr prefix;
-		  unsigned long int bits;
-		  unsigned long int val;
-		  char *endp;
-
-		  listp = &labellist;
-		  lenp = &nlabellist;
-		  nullbitsp = &labellist_nullbits;
-
-		new_elem:
-		  bits = 128;
-		  __set_errno (0);
-		  cp = strchr (val1, '/');
-		  if (cp != NULL)
-		    *cp++ = '\0';
-		  if (inet_pton (AF_INET6, val1, &prefix)
-		      && (cp == NULL
-			  || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
-			  || errno != ERANGE)
-		      && *endp == '\0'
-		      && bits <= 128
-		      && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
-			  || errno != ERANGE)
-		      && *endp == '\0'
-		      && val <= INT_MAX)
+		  if (!add_prefixlist (&labellist, &nlabellist,
+				       &labellist_nullbits, val1, val2, &cp))
 		    {
-		      struct prefixlist *newp = malloc (sizeof (*newp));
-		      if (newp == NULL)
-			{
-			  free (line);
-			  fclose (fp);
-			  goto no_file;
-			}
-
-		      memcpy (&newp->entry.prefix, &prefix, sizeof (prefix));
-		      newp->entry.bits = bits;
-		      newp->entry.val = val;
-		      newp->next = *listp;
-		      *listp = newp;
-		      ++*lenp;
-		      *nullbitsp |= bits == 0;
+		      free (line);
+		      fclose (fp);
+		      goto no_file;
 		    }
 		}
 	      break;
@@ -1926,27 +1948,14 @@ gaiconf_init (void)
 			  && *endp == '\0'
 			  && val <= INT_MAX)
 			{
-			  struct scopelist *newp;
-			new_scope:
-			  newp = malloc (sizeof (*newp));
-			  if (newp == NULL)
+			  if (!add_scopelist (&scopelist, &nscopelist,
+					      &scopelist_nullbits, &prefix,
+					      bits, val))
 			    {
 			      free (line);
 			      fclose (fp);
 			      goto no_file;
 			    }
-
-			  newp->entry.netmask = htonl (bits != 96
-						       ? (0xffffffff
-							  << (128 - bits))
-						       : 0);
-			  newp->entry.addr32 = (prefix.s6_addr32[3]
-						& newp->entry.netmask);
-			  newp->entry.scope = val;
-			  newp->next = scopelist;
-			  scopelist = newp;
-			  ++nscopelist;
-			  scopelist_nullbits |= bits == 96;
 			}
 		    }
 		  else if (inet_pton (AF_INET, val1, &prefix.s6_addr32[3])
@@ -1960,8 +1969,14 @@ gaiconf_init (void)
 			   && *endp == '\0'
 			   && val <= INT_MAX)
 		    {
-		      bits += 96;
-		      goto new_scope;
+		      if (!add_scopelist (&scopelist, &nscopelist,
+					  &scopelist_nullbits, &prefix,
+					  bits + 96, val))
+			{
+			  free (line);
+			  fclose (fp);
+			  goto no_file;
+			}
 		    }
 		}
 	      break;
@@ -1969,10 +1984,14 @@ gaiconf_init (void)
 	    case 10:
 	      if (strcmp (cmd, "precedence") == 0)
 		{
-		  listp = &precedencelist;
-		  lenp = &nprecedencelist;
-		  nullbitsp = &precedencelist_nullbits;
-		  goto new_elem;
+		  if (!add_prefixlist (&precedencelist, &nprecedencelist,
+				       &precedencelist_nullbits, val1, val2,
+				       &cp))
+		    {
+		      free (line);
+		      fclose (fp);
+		      goto no_file;
+		    }
 		}
 	      break;
 	    }
-- 
2.35.1

Re: [PATCH 1/3] gaiconf_init: Refactor some bits for readability

[DJ Delorie]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> Split out line processing for `label`, `precedence` and `scopev4` into
> separate functions instead of the gotos.

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

> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> +static bool
> +add_prefixlist (struct prefixlist **listp, size_t *lenp, bool *nullbitsp,
> +		char *val1, char *val2, char **pos)
> +{
> +  struct in6_addr prefix;
> +  unsigned long int bits;
> +  unsigned long int val;
> +  char *endp;
> +
> +  bits = 128;
> +  __set_errno (0);
> +  char *cp = strchr (val1, '/');
> +  if (cp != NULL)
> +    *cp++ = '\0';
> +  *pos = cp;
> +  if (inet_pton (AF_INET6, val1, &prefix)
> +      && (cp == NULL
> +	  || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
> +	  || errno != ERANGE)
> +      && *endp == '\0'
> +      && bits <= 128
> +      && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
> +	  || errno != ERANGE)
> +      && *endp == '\0'
> +      && val <= INT_MAX)
> +    {
> +      struct prefixlist *newp = malloc (sizeof (*newp));
> +      if (newp == NULL)
> +	return false;
> +
> +      memcpy (&newp->entry.prefix, &prefix, sizeof (prefix));
> +      newp->entry.bits = bits;
> +      newp->entry.val = val;
> +      newp->next = *listp;
> +      *listp = newp;
> +      ++*lenp;
> +      *nullbitsp |= bits == 0;
> +    }
> +  return true;
> +}

Matches removed code, ok.

> +static bool
> +add_scopelist (struct scopelist **listp, size_t *lenp, bool *nullbitsp,
> +	       const struct in6_addr *prefixp, unsigned long int bits,
> +	       unsigned long int val)
> +{
> +  struct scopelist *newp = malloc (sizeof (*newp));
> +  if (newp == NULL)
> +    return false;
> +
> +  newp->entry.netmask = htonl (bits != 96 ? (0xffffffff << (128 - bits)) : 0);
> +  newp->entry.addr32 = (prefixp->s6_addr32[3] & newp->entry.netmask);
> +  newp->entry.scope = val;
> +  newp->next = *listp;
> +  *listp = newp;
> +  ++*lenp;
> +  *nullbitsp |= bits == 96;
> +
> +  return true;
> +}

matches removed code, ok.

>  static void
>  gaiconf_init (void)
> @@ -1836,55 +1896,17 @@ gaiconf_init (void)
>  	  /*  Ignore the rest of the line.  */
>  	  *cp = '\0';
>  
> -	  struct prefixlist **listp;
> -	  size_t *lenp;
> -	  bool *nullbitsp;
>  	  switch (cmdlen)
>  	    {
>  	    case 5:
>  	      if (strcmp (cmd, "label") == 0)
>  		{
> +		  if (!add_prefixlist (&labellist, &nlabellist,
> +				       &labellist_nullbits, val1, val2, &cp))
>  		    {
> +		      free (line);
> +		      fclose (fp);
> +		      goto no_file;
>  		    }

Ok.

> -		  struct in6_addr prefix;
> -		  unsigned long int bits;
> -		  unsigned long int val;
> -		  char *endp;
> -
> -		  listp = &labellist;
> -		  lenp = &nlabellist;
> -		  nullbitsp = &labellist_nullbits;
> -
> -		new_elem:
> -		  bits = 128;
> -		  __set_errno (0);
> -		  cp = strchr (val1, '/');
> -		  if (cp != NULL)
> -		    *cp++ = '\0';
> -		  if (inet_pton (AF_INET6, val1, &prefix)
> -		      && (cp == NULL
> -			  || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
> -			  || errno != ERANGE)
> -		      && *endp == '\0'
> -		      && bits <= 128
> -		      && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
> -			  || errno != ERANGE)
> -		      && *endp == '\0'
> -		      && val <= INT_MAX)
>  		    {
> -		      struct prefixlist *newp = malloc (sizeof (*newp));
> -		      if (newp == NULL)
> -			{
> -			  free (line);
> -			  fclose (fp);
> -			  goto no_file;
> -			}
> -
> -		      memcpy (&newp->entry.prefix, &prefix, sizeof (prefix));
> -		      newp->entry.bits = bits;
> -		      newp->entry.val = val;
> -		      newp->next = *listp;
> -		      *listp = newp;
> -		      ++*lenp;
> -		      *nullbitsp |= bits == 0;

Matches extracted code, ok.

>  		    }
>  		}
>  	      break;
> @@ -1926,27 +1948,14 @@ gaiconf_init (void)
>  			  && *endp == '\0'
>  			  && val <= INT_MAX)
>  			{

> +			  if (!add_scopelist (&scopelist, &nscopelist,
> +					      &scopelist_nullbits, &prefix,
> +					      bits, val))

Ok.

>  			    {
>  			      free (line);
>  			      fclose (fp);
>  			      goto no_file;
>  			    }
> -			  struct scopelist *newp;
> -			new_scope:
> -			  newp = malloc (sizeof (*newp));
> -			  if (newp == NULL)
> - ...
> -
> -			  newp->entry.netmask = htonl (bits != 96
> -						       ? (0xffffffff
> -							  << (128 - bits))
> -						       : 0);
> -			  newp->entry.addr32 = (prefix.s6_addr32[3]
> -						& newp->entry.netmask);
> -			  newp->entry.scope = val;
> -			  newp->next = scopelist;
> -			  scopelist = newp;
> -			  ++nscopelist;
> -			  scopelist_nullbits |= bits == 96;

Matches extracted code, ok.

>  		    }
>  		  else if (inet_pton (AF_INET, val1, &prefix.s6_addr32[3])
> @@ -1960,8 +1969,14 @@ gaiconf_init (void)
>  			   && *endp == '\0'
>  			   && val <= INT_MAX)
>  		    {
> -		      bits += 96;
> -		      goto new_scope;
> +		      if (!add_scopelist (&scopelist, &nscopelist,
> +					  &scopelist_nullbits, &prefix,
> +					  bits + 96, val))
> +			{
> +			  free (line);
> +			  fclose (fp);
> +			  goto no_file;
> +			}

Ok.

> @@ -1969,10 +1984,14 @@ gaiconf_init (void)
>  	    case 10:
>  	      if (strcmp (cmd, "precedence") == 0)
>  		{
> -		  listp = &precedencelist;
> -		  lenp = &nprecedencelist;
> -		  nullbitsp = &precedencelist_nullbits;
> -		  goto new_elem;
> +		  if (!add_prefixlist (&precedencelist, &nprecedencelist,
> +				       &precedencelist_nullbits, val1, val2,
> +				       &cp))
> +		    {
> +		      free (line);
> +		      fclose (fp);
> +		      goto no_file;
> +		    }

Ok.

[PATCH 2/3] gai_init: Avoid jumping from if condition to its else counterpart

[Siddhesh Poyarekar]

Clean up another antipattern where code flows from an if condition to
its else counterpart with a goto.

Most of the change in this patch is whitespace-only; a `git diff -b`
ought to show the actual logic changes.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 sysdeps/posix/getaddrinfo.c | 516 ++++++++++++++++++------------------
 1 file changed, 257 insertions(+), 259 deletions(-)

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 95a61d7238..4e947024ec 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -1836,142 +1836,122 @@ gaiconf_init (void)
   bool scopelist_nullbits = false;
 
   FILE *fp = fopen (GAICONF_FNAME, "rce");
-  if (fp != NULL)
+  if (fp == NULL)
+    goto no_file;
+
+  struct __stat64_t64 st;
+  if (__fstat64_time64 (fileno (fp), &st) != 0)
     {
-      struct __stat64_t64 st;
-      if (__fstat64_time64 (fileno (fp), &st) != 0)
-	{
-	  fclose (fp);
-	  goto no_file;
-	}
+      fclose (fp);
+      goto no_file;
+    }
 
-      char *line = NULL;
-      size_t linelen = 0;
+  char *line = NULL;
+  size_t linelen = 0;
 
-      __fsetlocking (fp, FSETLOCKING_BYCALLER);
+  __fsetlocking (fp, FSETLOCKING_BYCALLER);
 
-      while (!feof_unlocked (fp))
+  while (!feof_unlocked (fp))
+    {
+      ssize_t n = __getline (&line, &linelen, fp);
+      if (n <= 0)
+	break;
+
+      /* Handle comments.  No escaping possible so this is easy.  */
+      char *cp = strchr (line, '#');
+      if (cp != NULL)
+	*cp = '\0';
+
+      cp = line;
+      while (isspace (*cp))
+	++cp;
+
+      char *cmd = cp;
+      while (*cp != '\0' && !isspace (*cp))
+	++cp;
+      size_t cmdlen = cp - cmd;
+
+      if (*cp != '\0')
+	*cp++ = '\0';
+      while (isspace (*cp))
+	++cp;
+
+      char *val1 = cp;
+      while (*cp != '\0' && !isspace (*cp))
+	++cp;
+      size_t val1len = cp - cmd;
+
+      /* We always need at least two values.  */
+      if (val1len == 0)
+	continue;
+
+      if (*cp != '\0')
+	*cp++ = '\0';
+      while (isspace (*cp))
+	++cp;
+
+      char *val2 = cp;
+      while (*cp != '\0' && !isspace (*cp))
+	++cp;
+
+      /*  Ignore the rest of the line.  */
+      *cp = '\0';
+
+      switch (cmdlen)
 	{
-	  ssize_t n = __getline (&line, &linelen, fp);
-	  if (n <= 0)
-	    break;
-
-	  /* Handle comments.  No escaping possible so this is easy.  */
-	  char *cp = strchr (line, '#');
-	  if (cp != NULL)
-	    *cp = '\0';
-
-	  cp = line;
-	  while (isspace (*cp))
-	    ++cp;
-
-	  char *cmd = cp;
-	  while (*cp != '\0' && !isspace (*cp))
-	    ++cp;
-	  size_t cmdlen = cp - cmd;
-
-	  if (*cp != '\0')
-	    *cp++ = '\0';
-	  while (isspace (*cp))
-	    ++cp;
-
-	  char *val1 = cp;
-	  while (*cp != '\0' && !isspace (*cp))
-	    ++cp;
-	  size_t val1len = cp - cmd;
-
-	  /* We always need at least two values.  */
-	  if (val1len == 0)
-	    continue;
-
-	  if (*cp != '\0')
-	    *cp++ = '\0';
-	  while (isspace (*cp))
-	    ++cp;
-
-	  char *val2 = cp;
-	  while (*cp != '\0' && !isspace (*cp))
-	    ++cp;
-
-	  /*  Ignore the rest of the line.  */
-	  *cp = '\0';
-
-	  switch (cmdlen)
+	case 5:
+	  if (strcmp (cmd, "label") == 0)
 	    {
-	    case 5:
-	      if (strcmp (cmd, "label") == 0)
+	      if (!add_prefixlist (&labellist, &nlabellist,
+				   &labellist_nullbits, val1, val2, &cp))
 		{
-		  if (!add_prefixlist (&labellist, &nlabellist,
-				       &labellist_nullbits, val1, val2, &cp))
-		    {
-		      free (line);
-		      fclose (fp);
-		      goto no_file;
-		    }
+		  free (line);
+		  fclose (fp);
+		  goto no_file;
 		}
-	      break;
+	    }
+	  break;
 
-	    case 6:
-	      if (strcmp (cmd, "reload") == 0)
-		{
-		  gaiconf_reload_flag = strcmp (val1, "yes") == 0;
-		  if (gaiconf_reload_flag)
-		    gaiconf_reload_flag_ever_set = 1;
-		}
-	      break;
+	case 6:
+	  if (strcmp (cmd, "reload") == 0)
+	    {
+	      gaiconf_reload_flag = strcmp (val1, "yes") == 0;
+	      if (gaiconf_reload_flag)
+		gaiconf_reload_flag_ever_set = 1;
+	    }
+	  break;
 
-	    case 7:
-	      if (strcmp (cmd, "scopev4") == 0)
+	case 7:
+	  if (strcmp (cmd, "scopev4") == 0)
+	    {
+	      struct in6_addr prefix;
+	      unsigned long int bits;
+	      unsigned long int val;
+	      char *endp;
+
+	      bits = 32;
+	      __set_errno (0);
+	      cp = strchr (val1, '/');
+	      if (cp != NULL)
+		*cp++ = '\0';
+	      if (inet_pton (AF_INET6, val1, &prefix))
 		{
-		  struct in6_addr prefix;
-		  unsigned long int bits;
-		  unsigned long int val;
-		  char *endp;
-
-		  bits = 32;
-		  __set_errno (0);
-		  cp = strchr (val1, '/');
-		  if (cp != NULL)
-		    *cp++ = '\0';
-		  if (inet_pton (AF_INET6, val1, &prefix))
-		    {
-		      bits = 128;
-		      if (IN6_IS_ADDR_V4MAPPED (&prefix)
-			  && (cp == NULL
-			      || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
-			      || errno != ERANGE)
-			  && *endp == '\0'
-			  && bits >= 96
-			  && bits <= 128
-			  && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
-			      || errno != ERANGE)
-			  && *endp == '\0'
-			  && val <= INT_MAX)
-			{
-			  if (!add_scopelist (&scopelist, &nscopelist,
-					      &scopelist_nullbits, &prefix,
-					      bits, val))
-			    {
-			      free (line);
-			      fclose (fp);
-			      goto no_file;
-			    }
-			}
-		    }
-		  else if (inet_pton (AF_INET, val1, &prefix.s6_addr32[3])
-			   && (cp == NULL
-			       || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
-			       || errno != ERANGE)
-			   && *endp == '\0'
-			   && bits <= 32
-			   && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
-			       || errno != ERANGE)
-			   && *endp == '\0'
-			   && val <= INT_MAX)
+		  bits = 128;
+		  if (IN6_IS_ADDR_V4MAPPED (&prefix)
+		      && (cp == NULL
+			  || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
+			  || errno != ERANGE)
+		      && *endp == '\0'
+		      && bits >= 96
+		      && bits <= 128
+		      && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
+			  || errno != ERANGE)
+		      && *endp == '\0'
+		      && val <= INT_MAX)
 		    {
 		      if (!add_scopelist (&scopelist, &nscopelist,
 					  &scopelist_nullbits, &prefix,
-					  bits + 96, val))
+					  bits, val))
 			{
 			  free (line);
 			  fclose (fp);
@@ -1979,173 +1959,191 @@ gaiconf_init (void)
 			}
 		    }
 		}
-	      break;
-
-	    case 10:
-	      if (strcmp (cmd, "precedence") == 0)
+	      else if (inet_pton (AF_INET, val1, &prefix.s6_addr32[3])
+		       && (cp == NULL
+			   || (bits = strtoul (cp, &endp, 10)) != ULONG_MAX
+			   || errno != ERANGE)
+		       && *endp == '\0'
+		       && bits <= 32
+		       && ((val = strtoul (val2, &endp, 10)) != ULONG_MAX
+			   || errno != ERANGE)
+		       && *endp == '\0'
+		       && val <= INT_MAX)
 		{
-		  if (!add_prefixlist (&precedencelist, &nprecedencelist,
-				       &precedencelist_nullbits, val1, val2,
-				       &cp))
+		  if (!add_scopelist (&scopelist, &nscopelist,
+				      &scopelist_nullbits, &prefix,
+				      bits + 96, val))
 		    {
 		      free (line);
 		      fclose (fp);
 		      goto no_file;
 		    }
 		}
-	      break;
-	    }
-	}
-
-      free (line);
-
-      fclose (fp);
-
-      /* Create the array for the labels.  */
-      struct prefixentry *new_labels;
-      if (nlabellist > 0)
-	{
-	  if (!labellist_nullbits)
-	    ++nlabellist;
-	  new_labels = malloc (nlabellist * sizeof (*new_labels));
-	  if (new_labels == NULL)
-	    goto no_file;
-
-	  int i = nlabellist;
-	  if (!labellist_nullbits)
-	    {
-	      --i;
-	      memset (&new_labels[i].prefix, '\0', sizeof (struct in6_addr));
-	      new_labels[i].bits = 0;
-	      new_labels[i].val = 1;
 	    }
+	  break;
 
-	  struct prefixlist *l = labellist;
-	  while (i-- > 0)
+	case 10:
+	  if (strcmp (cmd, "precedence") == 0)
 	    {
-	      new_labels[i] = l->entry;
-	      l = l->next;
+	      if (!add_prefixlist (&precedencelist, &nprecedencelist,
+				   &precedencelist_nullbits, val1, val2,
+				   &cp))
+		{
+		  free (line);
+		  fclose (fp);
+		  goto no_file;
+		}
 	    }
-	  free_prefixlist (labellist);
-	  labellist = NULL;
-
-	  /* Sort the entries so that the most specific ones are at
-	     the beginning.  */
-	  qsort (new_labels, nlabellist, sizeof (*new_labels), prefixcmp);
+	  break;
 	}
-      else
-	new_labels = (struct prefixentry *) default_labels;
-
-      struct prefixentry *new_precedence;
-      if (nprecedencelist > 0)
-	{
-	  if (!precedencelist_nullbits)
-	    ++nprecedencelist;
-	  new_precedence = malloc (nprecedencelist * sizeof (*new_precedence));
-	  if (new_precedence == NULL)
-	    {
-	      if (new_labels != default_labels)
-		free (new_labels);
-	      goto no_file;
-	    }
+    }
 
-	  int i = nprecedencelist;
-	  if (!precedencelist_nullbits)
-	    {
-	      --i;
-	      memset (&new_precedence[i].prefix, '\0',
-		      sizeof (struct in6_addr));
-	      new_precedence[i].bits = 0;
-	      new_precedence[i].val = 40;
-	    }
+  free (line);
 
-	  struct prefixlist *l = precedencelist;
-	  while (i-- > 0)
-	    {
-	      new_precedence[i] = l->entry;
-	      l = l->next;
-	    }
-	  free_prefixlist (precedencelist);
-	  precedencelist = NULL;
+  fclose (fp);
 
-	  /* Sort the entries so that the most specific ones are at
-	     the beginning.  */
-	  qsort (new_precedence, nprecedencelist, sizeof (*new_precedence),
-		 prefixcmp);
+  /* Create the array for the labels.  */
+  struct prefixentry *new_labels;
+  if (nlabellist > 0)
+    {
+      if (!labellist_nullbits)
+	++nlabellist;
+      new_labels = malloc (nlabellist * sizeof (*new_labels));
+      if (new_labels == NULL)
+	goto no_file;
+
+      int i = nlabellist;
+      if (!labellist_nullbits)
+	{
+	  --i;
+	  memset (&new_labels[i].prefix, '\0', sizeof (struct in6_addr));
+	  new_labels[i].bits = 0;
+	  new_labels[i].val = 1;
 	}
-      else
-	new_precedence = (struct prefixentry *) default_precedence;
 
-      struct scopeentry *new_scopes;
-      if (nscopelist > 0)
+      struct prefixlist *l = labellist;
+      while (i-- > 0)
 	{
-	  if (!scopelist_nullbits)
-	    ++nscopelist;
-	  new_scopes = malloc (nscopelist * sizeof (*new_scopes));
-	  if (new_scopes == NULL)
-	    {
-	      if (new_labels != default_labels)
-		free (new_labels);
-	      if (new_precedence != default_precedence)
-		free (new_precedence);
-	      goto no_file;
-	    }
-
-	  int i = nscopelist;
-	  if (!scopelist_nullbits)
-	    {
-	      --i;
-	      new_scopes[i].addr32 = 0;
-	      new_scopes[i].netmask = 0;
-	      new_scopes[i].scope = 14;
-	    }
+	  new_labels[i] = l->entry;
+	  l = l->next;
+	}
+      free_prefixlist (labellist);
+      labellist = NULL;
 
-	  struct scopelist *l = scopelist;
-	  while (i-- > 0)
-	    {
-	      new_scopes[i] = l->entry;
-	      l = l->next;
-	    }
-	  free_scopelist (scopelist);
+      /* Sort the entries so that the most specific ones are at
+	 the beginning.  */
+      qsort (new_labels, nlabellist, sizeof (*new_labels), prefixcmp);
+    }
+  else
+    new_labels = (struct prefixentry *) default_labels;
 
-	  /* Sort the entries so that the most specific ones are at
-	     the beginning.  */
-	  qsort (new_scopes, nscopelist, sizeof (*new_scopes),
-		 scopecmp);
+  struct prefixentry *new_precedence;
+  if (nprecedencelist > 0)
+    {
+      if (!precedencelist_nullbits)
+	++nprecedencelist;
+      new_precedence = malloc (nprecedencelist * sizeof (*new_precedence));
+      if (new_precedence == NULL)
+	{
+	  if (new_labels != default_labels)
+	    free (new_labels);
+	  goto no_file;
 	}
-      else
-	new_scopes = (struct scopeentry *) default_scopes;
-
-      /* Now we are ready to replace the values.  */
-      const struct prefixentry *old = labels;
-      labels = new_labels;
-      if (old != default_labels)
-	free ((void *) old);
 
-      old = precedence;
-      precedence = new_precedence;
-      if (old != default_precedence)
-	free ((void *) old);
+      int i = nprecedencelist;
+      if (!precedencelist_nullbits)
+	{
+	  --i;
+	  memset (&new_precedence[i].prefix, '\0',
+		  sizeof (struct in6_addr));
+	  new_precedence[i].bits = 0;
+	  new_precedence[i].val = 40;
+	}
 
-      const struct scopeentry *oldscope = scopes;
-      scopes = new_scopes;
-      if (oldscope != default_scopes)
-	free ((void *) oldscope);
+      struct prefixlist *l = precedencelist;
+      while (i-- > 0)
+	{
+	  new_precedence[i] = l->entry;
+	  l = l->next;
+	}
+      free_prefixlist (precedencelist);
+      precedencelist = NULL;
 
-      save_gaiconf_mtime (&st);
+      /* Sort the entries so that the most specific ones are at
+	 the beginning.  */
+      qsort (new_precedence, nprecedencelist, sizeof (*new_precedence),
+	     prefixcmp);
     }
   else
+    new_precedence = (struct prefixentry *) default_precedence;
+
+  struct scopeentry *new_scopes;
+  if (nscopelist > 0)
     {
-    no_file:
-      free_prefixlist (labellist);
-      free_prefixlist (precedencelist);
+      if (!scopelist_nullbits)
+	++nscopelist;
+      new_scopes = malloc (nscopelist * sizeof (*new_scopes));
+      if (new_scopes == NULL)
+	{
+	  if (new_labels != default_labels)
+	    free (new_labels);
+	  if (new_precedence != default_precedence)
+	    free (new_precedence);
+	  goto no_file;
+	}
+
+      int i = nscopelist;
+      if (!scopelist_nullbits)
+	{
+	  --i;
+	  new_scopes[i].addr32 = 0;
+	  new_scopes[i].netmask = 0;
+	  new_scopes[i].scope = 14;
+	}
+
+      struct scopelist *l = scopelist;
+      while (i-- > 0)
+	{
+	  new_scopes[i] = l->entry;
+	  l = l->next;
+	}
       free_scopelist (scopelist);
 
-      /* If we previously read the file but it is gone now, free the
-	 old data and use the builtin one.  Leave the reload flag
-	 alone.  */
-      fini ();
+      /* Sort the entries so that the most specific ones are at
+	 the beginning.  */
+      qsort (new_scopes, nscopelist, sizeof (*new_scopes),
+	     scopecmp);
     }
+  else
+    new_scopes = (struct scopeentry *) default_scopes;
+
+  /* Now we are ready to replace the values.  */
+  const struct prefixentry *old = labels;
+  labels = new_labels;
+  if (old != default_labels)
+    free ((void *) old);
+
+  old = precedence;
+  precedence = new_precedence;
+  if (old != default_precedence)
+    free ((void *) old);
+
+  const struct scopeentry *oldscope = scopes;
+  scopes = new_scopes;
+  if (oldscope != default_scopes)
+    free ((void *) oldscope);
+
+  save_gaiconf_mtime (&st);
+  return;
+
+no_file:
+  free_prefixlist (labellist);
+  free_prefixlist (precedencelist);
+  free_scopelist (scopelist);
+
+  /* If we previously read the file but it is gone now, free the old data and
+     use the builtin one.  Leave the reload flag alone.  */
+  fini ();
 }
 
 
-- 
2.35.1

Re: [PATCH 2/3] gai_init: Avoid jumping from if condition to its else counterpart

[DJ Delorie]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> Clean up another antipattern where code flows from an if condition to
> its else counterpart with a goto.
>
> Most of the change in this patch is whitespace-only; a `git diff -b`
> ought to show the actual logic changes.

Re-diffed as "diff -EZdbwpU5" and reviewing that...

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

> --- before.txt	2022-03-17 19:42:35.903049857 -0400
> +++ after.txt	2022-03-17 19:42:49.976581858 -0400
> @@ -1834,12 +1834,13 @@ gaiconf_init (void)
>    struct scopelist *scopelist =  NULL;
>    size_t nscopelist = 0;
>    bool scopelist_nullbits = false;
>  
>    FILE *fp = fopen (GAICONF_FNAME, "rce");
> +  if (fp == NULL)
> +    goto no_file;
> +
> -  if (fp != NULL)
> -    {

Ok.

>        struct __stat64_t64 st;
>        if (__fstat64_time64 (fileno (fp), &st) != 0)
>  	{
>  	  fclose (fp);
>  	  goto no_file;
> @@ -2131,24 +2132,21 @@ gaiconf_init (void)
>        scopes = new_scopes;
>        if (oldscope != default_scopes)
>  	free ((void *) oldscope);
>  
>        save_gaiconf_mtime (&st);
> -    }
> -  else
> -    {
> +  return;
> +

Ok.

>      no_file:
>        free_prefixlist (labellist);
>        free_prefixlist (precedencelist);
>        free_scopelist (scopelist);
>  
> -      /* If we previously read the file but it is gone now, free the
> -	 old data and use the builtin one.  Leave the reload flag
> -	 alone.  */
> +  /* If we previously read the file but it is gone now, free the old data and
> +     use the builtin one.  Leave the reload flag alone.  */
>        fini ();
>      }
> -}

Ok.

[PATCH 3/3] getaddrinfo: Refactor code for readability

[Siddhesh Poyarekar]

The close_retry goto jump is confusing and clumsy to read, so refactor
the code a bit to make it easier to follow.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 sysdeps/posix/getaddrinfo.c | 46 +++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 4e947024ec..5556876108 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -2156,6 +2156,37 @@ gaiconf_reload (void)
     gaiconf_init ();
 }
 
+static bool
+try_connect (int *fdp, int *afp, struct sockaddr_in6 *source_addrp,
+	     const struct sockaddr *addr, socklen_t addrlen, int family)
+{
+  int fd = *fdp;
+  int af = *afp;
+  socklen_t sl = sizeof (*source_addrp);
+  bool retry = false;
+
+  do
+    {
+      if (fd != -1 && __connect (fd, addr, addrlen) == 0
+	  && __getsockname (fd, (struct sockaddr *) source_addrp, &sl) == 0)
+	return true;
+      else if (errno == EAFNOSUPPORT && af == AF_INET6 && family == AF_INET)
+	{
+	  /* This could mean IPv6 sockets are IPv6-only.  */
+	  if (fd != -1)
+	    __close_nocancel_nostatus (fd);
+	  *afp = af = AF_INET;
+	  *fdp = fd = __socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC,
+				IPPROTO_IP);
+	  retry = true;
+	}
+      else
+	return false;
+    }
+  while (retry);
+
+  __builtin_unreachable ();
+}
 
 int
 getaddrinfo (const char *name, const char *service,
@@ -2346,7 +2377,6 @@ getaddrinfo (const char *name, const char *service,
 	      if (fd == -1 || (af == AF_INET && q->ai_family == AF_INET6))
 		{
 		  if (fd != -1)
-		  close_retry:
 		    __close_nocancel_nostatus (fd);
 		  af = q->ai_family;
 		  fd = __socket (af, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_IP);
@@ -2358,14 +2388,10 @@ getaddrinfo (const char *name, const char *service,
 		  __connect (fd, &sa, sizeof (sa));
 		}
 
-	      socklen_t sl = sizeof (results[i].source_addr);
-	      if (fd != -1
-		  && __connect (fd, q->ai_addr, q->ai_addrlen) == 0
-		  && __getsockname (fd,
-				    (struct sockaddr *) &results[i].source_addr,
-				    &sl) == 0)
+	      if (try_connect (&fd, &af, &results[i].source_addr, q->ai_addr,
+			       q->ai_addrlen, q->ai_family))
 		{
-		  results[i].source_addr_len = sl;
+		  results[i].source_addr_len = sizeof (results[i].source_addr);
 		  results[i].got_source_addr = true;
 
 		  if (in6ai != NULL)
@@ -2430,10 +2456,6 @@ getaddrinfo (const char *name, const char *service,
 		      results[i].source_addr_len = sizeof (struct sockaddr_in);
 		    }
 		}
-	      else if (errno == EAFNOSUPPORT && af == AF_INET6
-		       && q->ai_family == AF_INET)
-		/* This could mean IPv6 sockets are IPv6-only.  */
-		goto close_retry;
 	      else
 		/* Just make sure that if we have to process the same
 		   address again we do not copy any memory.  */
-- 
2.35.1

Re: [PATCH 3/3] getaddrinfo: Refactor code for readability

[DJ Delorie]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> The close_retry goto jump is confusing and clumsy to read, so refactor
> the code a bit to make it easier to follow.

LGTM with or without the tweak noted below.

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

> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c

> +static bool
> +try_connect (int *fdp, int *afp, struct sockaddr_in6 *source_addrp,
> +	     const struct sockaddr *addr, socklen_t addrlen, int family)
> +{
> +  int fd = *fdp;
> +  int af = *afp;
> +  socklen_t sl = sizeof (*source_addrp);
> +  bool retry = false;
> +
> +  do
> +    {
> +      if (fd != -1 && __connect (fd, addr, addrlen) == 0
> +	  && __getsockname (fd, (struct sockaddr *) source_addrp, &sl) == 0)
> +	return true;
> +      else if (errno == EAFNOSUPPORT && af == AF_INET6 && family == AF_INET)
> +	{
> +	  /* This could mean IPv6 sockets are IPv6-only.  */
> +	  if (fd != -1)
> +	    __close_nocancel_nostatus (fd);
> +	  *afp = af = AF_INET;
> +	  *fdp = fd = __socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC,
> +				IPPROTO_IP);
> +	  retry = true;
> +	}
> +      else
> +	return false;
> +    }
> +  while (retry);
> +
> +  __builtin_unreachable ();
> +}

The do/while loop must follow one of three paths: return true,
retry=true, or return false.  The builtin_unreachable() agrees.  Thus,
the whole retry logic is unneeded and can be removed, using a
do/while(1) loop instead.

I'll OK this as-is though, but if your task is to simplify... then
simplify :-)

> @@ -2346,7 +2377,6 @@ getaddrinfo (const char *name, const char *service,
>  	      if (fd == -1 || (af == AF_INET && q->ai_family == AF_INET6))
>  		{
>  		  if (fd != -1)
> -		  close_retry:

Ok.

> +	      if (try_connect (&fd, &af, &results[i].source_addr, q->ai_addr,
> +			       q->ai_addrlen, q->ai_family))
> -	      socklen_t sl = sizeof (results[i].source_addr);
> -	      if (fd != -1
> -		  && __connect (fd, q->ai_addr, q->ai_addrlen) == 0
> -		  && __getsockname (fd,
> -				    (struct sockaddr *) &results[i].source_addr,
> -				    &sl) == 0)

Matches moved code, ok.

> -		  results[i].source_addr_len = sl;
> +		  results[i].source_addr_len = sizeof (results[i].source_addr);

Ok.

> -	      else if (errno == EAFNOSUPPORT && af == AF_INET6
> -		       && q->ai_family == AF_INET)
> -		/* This could mean IPv6 sockets are IPv6-only.  */
> -		goto close_retry;

Matches moved code, ok.

[PATCH v2] getaddrinfo: Refactor code for readability

[Siddhesh Poyarekar]

The close_retry goto jump is confusing and clumsy to read, so refactor
the code a bit to make it easier to follow.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 sysdeps/posix/getaddrinfo.c | 45 +++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index aa34de6591..bcff909b2f 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -2246,6 +2246,36 @@ gaiconf_reload (void)
     gaiconf_init ();
 }
 
+static bool
+try_connect (int *fdp, int *afp, struct sockaddr_in6 *source_addrp,
+	     const struct sockaddr *addr, socklen_t addrlen, int family)
+{
+  int fd = *fdp;
+  int af = *afp;
+  socklen_t sl = sizeof (*source_addrp);
+
+  while (true)
+    {
+      if (fd != -1 && __connect (fd, addr, addrlen) == 0
+	  && __getsockname (fd, (struct sockaddr *) source_addrp, &sl) == 0)
+	return true;
+
+      if (errno == EAFNOSUPPORT && af == AF_INET6 && family == AF_INET)
+	{
+	  /* This could mean IPv6 sockets are IPv6-only.  */
+	  if (fd != -1)
+	    __close_nocancel_nostatus (fd);
+	  *afp = af = AF_INET;
+	  *fdp = fd = __socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC,
+				IPPROTO_IP);
+	  continue;
+	}
+
+      return false;
+    }
+
+  __builtin_unreachable ();
+}
 
 int
 getaddrinfo (const char *name, const char *service,
@@ -2436,7 +2466,6 @@ getaddrinfo (const char *name, const char *service,
 	      if (fd == -1 || (af == AF_INET && q->ai_family == AF_INET6))
 		{
 		  if (fd != -1)
-		  close_retry:
 		    __close_nocancel_nostatus (fd);
 		  af = q->ai_family;
 		  fd = __socket (af, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_IP);
@@ -2448,14 +2477,10 @@ getaddrinfo (const char *name, const char *service,
 		  __connect (fd, &sa, sizeof (sa));
 		}
 
-	      socklen_t sl = sizeof (results[i].source_addr);
-	      if (fd != -1
-		  && __connect (fd, q->ai_addr, q->ai_addrlen) == 0
-		  && __getsockname (fd,
-				    (struct sockaddr *) &results[i].source_addr,
-				    &sl) == 0)
+	      if (try_connect (&fd, &af, &results[i].source_addr, q->ai_addr,
+			       q->ai_addrlen, q->ai_family))
 		{
-		  results[i].source_addr_len = sl;
+		  results[i].source_addr_len = sizeof (results[i].source_addr);
 		  results[i].got_source_addr = true;
 
 		  if (in6ai != NULL)
@@ -2520,10 +2545,6 @@ getaddrinfo (const char *name, const char *service,
 		      results[i].source_addr_len = sizeof (struct sockaddr_in);
 		    }
 		}
-	      else if (errno == EAFNOSUPPORT && af == AF_INET6
-		       && q->ai_family == AF_INET)
-		/* This could mean IPv6 sockets are IPv6-only.  */
-		goto close_retry;
 	      else
 		/* Just make sure that if we have to process the same
 		   address again we do not copy any memory.  */
-- 
2.35.1

Re: [PATCH v2] getaddrinfo: Refactor code for readability

[DJ Delorie]

LGTM

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

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
>  
> +static bool
> +try_connect (int *fdp, int *afp, struct sockaddr_in6 *source_addrp,
> +	     const struct sockaddr *addr, socklen_t addrlen, int family)
> +{
> +  int fd = *fdp;
> +  int af = *afp;
> +  socklen_t sl = sizeof (*source_addrp);
> +
> +  while (true)
> +    {
> +      if (fd != -1 && __connect (fd, addr, addrlen) == 0
> +	  && __getsockname (fd, (struct sockaddr *) source_addrp, &sl) == 0)
> +	return true;
> +
> +      if (errno == EAFNOSUPPORT && af == AF_INET6 && family == AF_INET)
> +	{
> +	  /* This could mean IPv6 sockets are IPv6-only.  */
> +	  if (fd != -1)
> +	    __close_nocancel_nostatus (fd);
> +	  *afp = af = AF_INET;
> +	  *fdp = fd = __socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC,
> +				IPPROTO_IP);
> +	  continue;
> +	}
> +
> +      return false;
> +    }
> +
> +  __builtin_unreachable ();
> +}

Ok.

>  		  if (fd != -1)
> -		  close_retry:
>  		    __close_nocancel_nostatus (fd);

Ok.

> -	      socklen_t sl = sizeof (results[i].source_addr);
> -	      if (fd != -1
> -		  && __connect (fd, q->ai_addr, q->ai_addrlen) == 0
> -		  && __getsockname (fd,
> -				    (struct sockaddr *) &results[i].source_addr,
> -				    &sl) == 0)
> +	      if (try_connect (&fd, &af, &results[i].source_addr, q->ai_addr,
> +			       q->ai_addrlen, q->ai_family))

Ok.

> -		  results[i].source_addr_len = sl;
> +		  results[i].source_addr_len = sizeof (results[i].source_addr);

Ok.

> -	      else if (errno == EAFNOSUPPORT && af == AF_INET6
> -		       && q->ai_family == AF_INET)
> -		/* This could mean IPv6 sockets are IPv6-only.  */
> -		goto close_retry;

Ok.