[PATCH v5 1/4] Add system-wide tunables: ldconfig part

[Yury Khrustalev]

On Mon, Dec 04, 2023 at 08:48:18PM -0500, DJ Delorie wrote:
> 
> Adds support for reading /etc/tunables.conf

Overall I think this commit should be preceded with a refactoring of
reading ld config files to allow for also reading other types of
configs. The size of duplicated code is huge, IMO.

However, it's good that we have this version now as we can see what
might be required of the generalised version of existing code.

A couple of comments below, still reviewing the series...

> The file contains one line per tunable, like this:
> 
> glibc.foo.bar=14
> glibc.malloc.more=0
> 
> Additionally, each line can be prefixed with a single character
> that controls overridability by the GLIBC_TUNABLES env var:
> 
> !glibc.foo=0
>    ^ May be made more secure
> +glibc.foo=0
>    ^ May be overridden
> -glibc.foo=0
>    ^ May not be overridden

We need tests for all this. Let me know if you don't have time for
writing them, I think I can help with that, though I don't know how to
combine two patch series when one depends on the other.

> ...
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 396e97b5e7..42da891f5a 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile

OK

> ...
>
> diff --git a/elf/cache.c b/elf/cache.c
> index a6dc85dc0f..e798f7e2d8 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c

OK

> ...
>
> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index aca967403a..1bd81f4220 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c

OK

> ...
>
> diff --git a/elf/tunconf.c b/elf/tunconf.c
> new file mode 100644
> index 0000000000..ea547e95d5
> --- /dev/null
> +++ b/elf/tunconf.c
>
> ...
>
> +static void
> +add_tunable (char *line, const char *filename, int lineno)
> +{
>
> ...
>
> +  /* Look for the '=' separator.  */
> +  eq = strchr (line, '=');
> +  if (eq == NULL)
> +    {
> +      printf("%s:%d: syntax error, line ignored: `%s' (missing '=')\n",
> +	     filename, lineno, orig_line);

I think ldconfig.c uses error(...) function from error.h rather than
printf.

> ...
>
> +  for (i=0; i<TUNABLE_NAME_MAX; i++)
> +    if (strcmp (tunable_list[i].name, name) == 0)

The TUNABLE_NAME_MAX is not the length of tunable_list, you should use
something like

  enum { tunables_list_size = array_length (tunable_list) };

from elf/dl-tunables.c.

> ...
>
> +
> +/* Handle one word in an `include' line, a glob pattern of additional
> +   config files to read.  */
> +static void
> +parse_tunconf_include (const char *tunconfig_file, unsigned int lineno,
> +		       bool do_chroot, const char *pattern, const char *opt_chroot)

I still think like this should be shared code that handles config files
with clear interface and appropriate tests. If the rule of file
inclusion and look-up in xyz.conf.d are the same, we shouldn't have to
have another function to handle includes.

> ...
>
> diff --git a/elf/tunconf.h b/elf/tunconf.h
> new file mode 100644
> index 0000000000..a6c5f0dd9a
> --- /dev/null
> +++ b/elf/tunconf.h

OK

> ...
>
> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
> index 26f0c1e0bd..f438982b6f 100644
> --- a/sysdeps/generic/dl-cache.h
> +++ b/sysdeps/generic/dl-cache.h

OK

> ...
>
> diff --git a/sysdeps/generic/ldconfig.h b/sysdeps/generic/ldconfig.h
> index 5913bf40fc..da97774793 100644
> --- a/sysdeps/generic/ldconfig.h
> +++ b/sysdeps/generic/ldconfig.h

OK

Thanks,
Yury