[09/19] nptl: Use __pthread_attr_copy in pthread_setattr_default_np

Message ID 74f2d4c0a6cd892c70d693fff4bd75a65c4c2e27.1589884403.git.fweimer@redhat.com
State New
Headers show
Series
  • Signal mask for timer helper thread
Related show

Commit Message

Alejandro Colomar via Libc-alpha May 19, 2020, 10:44 a.m.
---
 nptl/pthread_setattr_default_np.c | 53 +++++++++----------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

-- 
2.25.4

Comments

Alejandro Colomar via Libc-alpha May 20, 2020, 2:48 p.m. | #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;

>  }

> 



-- 
Cheers,
Carlos.

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