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

[Arjun Shankar]

Hi DJ,

In print_extensions:

> +      /* Check that COUNT won't overflow our data block.  */
> +      assert (ext->sections[cache_extension_tag_tunables].base
> +             + ext->sections[cache_extension_tag_tunables].size
> +             >= (void *) & tec[count]);

Should we make it stricter with a check for exact size? Something like:

     sizeof (tunable_header_cached)
     + count * sizeof (tunable_entry_cached)
     == ext->sections[cache_extension_tag_tunables].size

I'm also wondering if it's worth checking for overflow before the
addition. Maybe it's overkill, but I wanted to mention it.

...
> @@ -498,6 +528,28 @@ write_extensions (int fd, uint32_t str_offset,
>        ext->sections[xid].size = hwcaps_size;
>      }
>
> +  struct tunable_header_cached *tunable_data;
> +  size_t tunable_size;
> +  size_t tunable_aligner = 0;
> +
> +  tunable_data = get_tunconf_ext (str_offset);

This is non-NULL even when there are no tunables, leading to a
superfluous extension section and data being written here.

...
> @@ -1089,6 +1101,9 @@ main (int argc, char **argv)
>    if (config_file == NULL)
>      config_file = LD_SO_CONF;
>
> +  if (tunconfig_file == NULL)
> +    tunconfig_file = TUNABLES_CONF;
> +
>    if (opt_print_cache)
>      {
>        if (opt_chroot != NULL)
> @@ -1164,6 +1179,8 @@ main (int argc, char **argv)
>
>    search_dirs ();
>
> +  parse_tunconf (tunconfig_file, opt_chroot);
> +

Looks like it could be guarded by opt_build_cache since nothing else
consumes the parsed data.

> +++ b/elf/tunconf.c
...
> +/* Declared in chroot_canon.c.  */
> +extern char *chroot_canon (const char *chroot, const char *name);

Looks like it's declared in sysdeps/generic/ldconfig.h.  Not very sure
if that gets overridden by some non-generic variant.

...
> +struct tunable_entry_int {
> +  struct stringtable_entry *name;
> +  struct stringtable_entry *value;
> +  TOP top;
> +  int tunable_id;
> +  int value_is_negative:1;
> +  int value_was_parsed:1;
> +  unsigned long long value_ull;
> +  signed long long value_sll;

These last four are never written to by ldconfig.

> +
> +  struct tunable_entry_int *next;
> +};
> +
> +struct tunable_entry_int *entry_list;
> +struct tunable_entry_int **entry_list_next = &entry_list;

This could be local to add_tunable. It doesn't appear to be used elsewhere.

In add_tunable:

> +  id = -1;
> +  for (i = 0; i < array_length (tunable_list); i ++)
> +    if (strcmp (tunable_list[i].name, name) == 0)
> +      {
> +       id = i;
> +       break;
> +      }

Here, when we don't find the tunable (id == -1), we store it anyway. This
makes sense since ldconfig might not know about a valid tunable. Is this
worth reporting to the user? A message can guard against situations where a
subtly misspelled or typo-ed tunable doesn't get silently ignored at caching
time as well as loading time.

> +
> +  entry = (struct tunable_entry_int *) xcalloc (sizeof (struct tunable_entry_int), 1);
> +  entry->name = cache_store_string (name);
> +  entry->value = cache_store_string (value);
> +  entry->tunable_id = id;
> +  entry->top = top;

The parsing of numeric tunable values into the remaining fields should
happen here, if at all.

> +struct tunable_header_cached *
> +get_tunconf_ext (uint32_t string_table_offset)
> +{
> +  struct tunable_entry_int *tei;
> +  struct tunable_header_cached *thc;
> +  size_t count;
> +  size_t size;
> +
> +  /* First, count the number of entries we have.  */
> +  tei = entry_list;
> +  count = 0;
> +  while (tei != NULL)
> +    {
> +      ++ count;
> +      tei = tei->next;
> +    }

Can we return early here with an empty/NULL result when count is 0?

> +  /* Allocate enough space for the whole cached block.  */
> +  size = sizeof (struct tunable_header_cached)
> +       + sizeof (struct tunable_entry_cached) * count;
> +  thc = (struct tunable_header_cached *) malloc (size);
> +
> +  if (thc == NULL)
> +    {
> +      error (0, 0, _("Unable to allocate %zu bytes in get_tunable_ext"), size);

Typo. Should be get_tunconf_ext.

> +      return NULL;
> +    }
> +
> +  /* Now, fill in the structures.  */
> +
> +  thc->signature = TUNCONF_SIGNATURE;
> +  thc->version = TUNCONF_VERSION;
> +  thc->num_tunables = count;
> +  thc->unused_1 = 0;
> +
> +  tei = entry_list;
> +  count = 0;
> +  while (tei != NULL)
> +    {
> +      struct tunable_entry_cached *tec;
> +
> +      tec = & ( thc->tunables[count] );
> +
> +      tec->flags = 0;
> +      if (tei->value_was_parsed)

This will always be false.

> +       tec->flags |= TUNCONF_FLAG_PARSED;
> +      if (tei->value_is_negative)

This as well.

Cheers,
Arjun