Re: [PATCH 4/8] elf: Let environment aliases override overridable system-wide tunables

LGTM
Reviewed-by: DJ Delorie <[email protected]>

Adhemerval Zanella <[email protected]> writes:
> +/* Records tunables that were set from GLIBC_TUNABLES during this call, so

I wonder if it would make sense to have one variable that had the
"origin priority" of the set value, from TUNABLE_UNSET through
TUNABLE_LOCKED, so a single comparison tells us if a given source can
change the value and bump the priority, replacing initialized, locked,
and the new set_by_env?

TUNABLE_UNSET
TUNABLE_GLOBAL
TUNABLE_LEGACY
TUNABLE_ENVIRON
TUNABLE_LOCKED

(something for after the release, as it's purely internal)

> +   that a legacy environment-variable alias does not override themi (the
> +   canonical GLIBC_TUNABLES form takes precedence over the aliases).
> +   A tunable that was set only from the system-wide cache is deliberately not
> +   recorded here, so an alias may still override an overridable cache default;
> +   a nonoverridable one remains protected by tunable_t::locked.  */
> +static bool tunable_set_by_env[tunables_list_size];

Ok.

>        if (!tunable_initialize (tunables[i].t, tunables[i].value,
>  			       tunables[i].len))
>  	parse_tunable_print_error (&tunables[i]);
> +      else
> +	/* GLIBC_TUNABLES set this tunable; a legacy alias must not
> +	   override it.  */
> +	tunable_set_by_env[i] = true;
>      }
>  }

Ok.

> @@ -498,9 +510,13 @@ __tunables_init (char **envp, char **argv)
>  
>    for (int i = 0; i < tunable_num_env_alias; i++)
>      {
> -      /* Skip over tunables that have either been set or already initialized.  */
> +      /* Skip aliases whose tunable was already set through GLIBC_TUNABLES,
> +	 which takes precedence over the alias.  A value coming only from the
> +	 system-wide cache does not block the alias here: an overridable cache
> +	 default may still be overridden, while a nonoverridable one is
> +	 protected by tunable_t::locked.  */
>        if (tunables_env_alias[i].t == NULL
> -	  || tunables_env_alias[i].t->initialized)
> +	  || tunable_set_by_env[tunable_env_alias_list[i]])
>  	continue;
>  

Ok.

> diff --git a/elf/tst-tunconf1.c b/elf/tst-tunconf1.c

> +  /* Interaction with legacy environment-variable aliases (MALLOC_*).  */
> +  int32_t mmap_max = TUNABLE_GET_FULL (glibc, malloc, mmap_max, int32_t, NULL);
> +  size_t top_pad = TUNABLE_GET_FULL (glibc, malloc, top_pad, size_t, NULL);
> +  size_t arena_max = TUNABLE_GET_FULL (glibc, malloc, arena_max, size_t, NULL);
> +
> +  /* Overridable cache default (100); the MALLOC_MMAP_MAX_ alias overrides
> +     it, just like GLIBC_TUNABLES would.  */
> +  printf("mmap_max is %d (should be 200, from MALLOC_MMAP_MAX_ alias)\n",
> +	 mmap_max);
> +  TEST_COMPARE (mmap_max, 200);
> +
> +  /* Nonoverridable cache default (100); the MALLOC_TOP_PAD_ alias must not
> +     override it.  */
> +  printf("top_pad is %ld (should be 100, from /etc nonoverridable)\n",
> +	 (long)top_pad);
> +  TEST_COMPARE ((long)top_pad, 100);
> +
> +  /* Set both by GLIBC_TUNABLES (300) and by the MALLOC_ARENA_MAX alias
> +     (400); the canonical GLIBC_TUNABLES form wins.  */
> +  printf("arena_max is %ld (should be 300, from GLIBC_TUNABLES)\n",
> +	 (long)arena_max);
> +  TEST_COMPARE ((long)arena_max, 300);

Ok.

> diff --git a/elf/tst-tunconf1.root/etc/tunables.conf b/elf/tst-tunconf1.root/etc/tunables.conf

> +# Interaction with legacy environment-variable aliases (MALLOC_*), checked
> +# in the test case:
> +#  - mmap_max: overridable cache default, must be overridable by the
> +#    MALLOC_MMAP_MAX_ alias just like by GLIBC_TUNABLES.
> +#  - top_pad: nonoverridable cache default, must NOT be overridable by the
> +#    MALLOC_TOP_PAD_ alias.
> +glibc.malloc.mmap_max=100
> +-glibc.malloc.top_pad=100

Ok.