[09/19] nptl: Use __pthread_attr_copy in pthread_setattr_default_np
Commit Message
---
nptl/pthread_setattr_default_np.c | 53 +++++++++----------------------
1 file changed, 15 insertions(+), 38 deletions(-)
Comments
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;
> }
>
@@ -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;
}