diff mbox series

[09/19] nptl: Use __pthread_attr_copy in pthread_setattr_default_np

Message ID 74f2d4c0a6cd892c70d693fff4bd75a65c4c2e27.1589884403.git.fweimer@redhat.com
State Committed
Headers show
Series Signal mask for timer helper thread | expand

Commit Message

Florian Weimer May 19, 2020, 10:44 a.m. UTC
---
 nptl/pthread_setattr_default_np.c | 53 +++++++++----------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

Comments

Carlos O'Donell May 20, 2020, 2:48 p.m. UTC | #1
On 5/19/20 6:44 AM, Florian Weimer via Libc-alpha wrote:
> ---
>  nptl/pthread_setattr_default_np.c | 53 +++++++++----------------------
>  1 file changed, 15 insertions(+), 38 deletions(-)
> 

OK for master.

Tested clean on x86_64 and i686.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/nptl/pthread_setattr_default_np.c b/nptl/pthread_setattr_default_np.c
> index d6a46624b5..eb5d24d3bd 100644
> --- a/nptl/pthread_setattr_default_np.c
> +++ b/nptl/pthread_setattr_default_np.c
> @@ -26,7 +26,6 @@ int
>  pthread_setattr_default_np (const pthread_attr_t *in)
>  {
>    const struct pthread_attr *real_in;
> -  struct pthread_attr attrs;

OK.

>    int ret;
>  
>    real_in = (struct pthread_attr *) in;
> @@ -58,49 +57,27 @@ pthread_setattr_default_np (const pthread_attr_t *in)
>    if (real_in->flags & ATTR_FLAG_STACKADDR)
>      return EINVAL;
>  
> -  attrs = *real_in;
> +  union pthread_attr_transparent temp;
> +  ret = __pthread_attr_copy (&temp.external, in);
> +  if (ret != 0)
> +    return ret;

OK. Make a deep copy and allocate as required. May fail with ENOMEM.

>  
> -  /* Now take the lock because we start writing into
> +  /* Now take the lock because we start accessing
>       __default_pthread_attr.  */
>    lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);

OK.

>  
> -  /* Free the cpuset if the input is 0.  Otherwise copy in the cpuset
> -     contents.  */
> -  size_t cpusetsize = attrs.cpusetsize;
> -  if (cpusetsize == 0)
> -    {
> -      free (__default_pthread_attr.cpuset);
> -      __default_pthread_attr.cpuset = NULL;
> -    }
> -  else if (cpusetsize == __default_pthread_attr.cpusetsize)
> -    {
> -      attrs.cpuset = __default_pthread_attr.cpuset;
> -      memcpy (attrs.cpuset, real_in->cpuset, cpusetsize);
> -    }
> -  else
> -    {
> -      /* This may look wrong at first sight, but it isn't.  We're freeing
> -	 __default_pthread_attr.cpuset and allocating to attrs.cpuset because
> -	 we'll copy over all of attr to __default_pthread_attr later.  */
> -      cpu_set_t *newp = realloc (__default_pthread_attr.cpuset,
> -				 cpusetsize);
> -
> -      if (newp == NULL)
> -	{
> -	  ret = ENOMEM;
> -	  goto out;
> -	}
> -
> -      attrs.cpuset = newp;
> -      memcpy (attrs.cpuset, real_in->cpuset, cpusetsize);
> -    }

OK. We don't need any of this code because we did a deep copy and will
subsequently destroy the attribute. I like the cleanup.

> +  /* Preserve the previous stack size (see above).  */
> +  if (temp.internal.stacksize == 0)
> +    temp.internal.stacksize = __default_pthread_attr.stacksize;

OK.

> +
> +  /* Destroy the old attribute structure because it will be
> +     overwritten.  */
> +  __pthread_attr_destroy ((pthread_attr_t *) &__default_pthread_attr);

OK.

>  
> -  /* We don't want to accidentally set the default stacksize to zero.  */
> -  if (attrs.stacksize == 0)
> -    attrs.stacksize = __default_pthread_attr.stacksize;
> -  __default_pthread_attr = attrs;
> +  /* __default_pthread_attr takes ownership, so do not free
> +     attrs.internal after this point.  */
> +  __default_pthread_attr = temp.internal;

OK.

>  
> - out:
>    lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>    return ret;
>  }
>
diff mbox series

Patch

diff --git a/nptl/pthread_setattr_default_np.c b/nptl/pthread_setattr_default_np.c
index d6a46624b5..eb5d24d3bd 100644
--- a/nptl/pthread_setattr_default_np.c
+++ b/nptl/pthread_setattr_default_np.c
@@ -26,7 +26,6 @@  int
 pthread_setattr_default_np (const pthread_attr_t *in)
 {
   const struct pthread_attr *real_in;
-  struct pthread_attr attrs;
   int ret;
 
   real_in = (struct pthread_attr *) in;
@@ -58,49 +57,27 @@  pthread_setattr_default_np (const pthread_attr_t *in)
   if (real_in->flags & ATTR_FLAG_STACKADDR)
     return EINVAL;
 
-  attrs = *real_in;
+  union pthread_attr_transparent temp;
+  ret = __pthread_attr_copy (&temp.external, in);
+  if (ret != 0)
+    return ret;
 
-  /* Now take the lock because we start writing into
+  /* Now take the lock because we start accessing
      __default_pthread_attr.  */
   lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
 
-  /* Free the cpuset if the input is 0.  Otherwise copy in the cpuset
-     contents.  */
-  size_t cpusetsize = attrs.cpusetsize;
-  if (cpusetsize == 0)
-    {
-      free (__default_pthread_attr.cpuset);
-      __default_pthread_attr.cpuset = NULL;
-    }
-  else if (cpusetsize == __default_pthread_attr.cpusetsize)
-    {
-      attrs.cpuset = __default_pthread_attr.cpuset;
-      memcpy (attrs.cpuset, real_in->cpuset, cpusetsize);
-    }
-  else
-    {
-      /* This may look wrong at first sight, but it isn't.  We're freeing
-	 __default_pthread_attr.cpuset and allocating to attrs.cpuset because
-	 we'll copy over all of attr to __default_pthread_attr later.  */
-      cpu_set_t *newp = realloc (__default_pthread_attr.cpuset,
-				 cpusetsize);
-
-      if (newp == NULL)
-	{
-	  ret = ENOMEM;
-	  goto out;
-	}
-
-      attrs.cpuset = newp;
-      memcpy (attrs.cpuset, real_in->cpuset, cpusetsize);
-    }
+  /* Preserve the previous stack size (see above).  */
+  if (temp.internal.stacksize == 0)
+    temp.internal.stacksize = __default_pthread_attr.stacksize;
+
+  /* Destroy the old attribute structure because it will be
+     overwritten.  */
+  __pthread_attr_destroy ((pthread_attr_t *) &__default_pthread_attr);
 
-  /* We don't want to accidentally set the default stacksize to zero.  */
-  if (attrs.stacksize == 0)
-    attrs.stacksize = __default_pthread_attr.stacksize;
-  __default_pthread_attr = attrs;
+  /* __default_pthread_attr takes ownership, so do not free
+     attrs.internal after this point.  */
+  __default_pthread_attr = temp.internal;
 
- out:
   lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
   return ret;
 }