Re: [PATCH 27/28] elf: Process glibc-hwcaps subdirectories in ldconfig

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella via Libc-alpha:

>> +/* Helper for sorting struct glibc_hwcaps_subdirectory elements by
>> +   name.  */
>> +static int
>> +assign_glibc_hwcaps_indices_compare (const void *l, const void *r)
>> +{
>> +  const struct glibc_hwcaps_subdirectory *left
>> +    = *(struct glibc_hwcaps_subdirectory **)l;
>> +  const struct glibc_hwcaps_subdirectory *right
>> +    = *(struct glibc_hwcaps_subdirectory **)r;
>> +  return strcmp (left->name->string, right->name->string);
>> +}
>> +
>
> Maybe:
>
>   strcmp (glibc_hwcaps_subdirectory_name (left),
>           glibc_hwcaps_subdirectory_name (right));

Fixed.

>> +/* Compute the section_index fields for all   */
>> +static void
>> +assign_glibc_hwcaps_indices (void)
>> +{
>> +  /* Convert the linked list into an array, so that we can use qsort.
>> +     Only copy the subdirectories which are actually used.  */
>> +  size_t count = glibc_hwcaps_count ();
>> +  struct glibc_hwcaps_subdirectory **array
>> +    = xmalloc (sizeof (*array) * count);
>> +  {
>> +    size_t i = 0;
>> +    for (struct glibc_hwcaps_subdirectory *p = hwcaps; p != NULL; p = p->next)
>> +      if (p->used)
>> +	{
>> +	  array[i] = p;
>> +	  ++i;
>> +	}
>> +    assert (i == count);
>
> Do we need this assert? I think it would make sense if hwcaps is modified
> concurrently, which does not seem the case.

It documents that the loop processed the entire array, consistent with
glibc_hwcaps_count.  Right now, the function is defined immediately
above, but that could change, and I think it is reasonable to capture
this dependency.

>> @@ -311,8 +442,22 @@ compare (const struct cache_entry *e1, const struct cache_entry *e2)
>>  	return 1;
>>        else if (e1->flags > e2->flags)
>>  	return -1;
>> +      /* Keep the glibc-hwcaps extension entries before the regular
>> +	 entries, and sort them by their names.  search_cache in
>> +	 dl-cache.c stops searching once the first non-extension entry
>> +	 is found, so the extension entries need to come first.  */
>> +      else if (e1->hwcaps != NULL && e2->hwcaps == NULL)
>> +	return -1;
>> +      else if (e1->hwcaps == NULL && e2->hwcaps != NULL)
>> +	return 1;
>> +      else if (e1->hwcaps != NULL && e2->hwcaps != NULL)
>> +	{
>> +	  res = strcmp (e1->hwcaps->name->string, e2->hwcaps->name->string);
>
> Maybe:
>
>   res = strcmp (glibc_hwcaps_subdirectory_name (e1->hwcaps),
>                 glibc_hwcaps_subdirectory_name (e2->hwcaps));

Fixed.

>> -/* Write the cache extensions to FD.  The extension directory is
>> -   assumed to be located at CACHE_EXTENSION_OFFSET.  */
>> +/* Write the cache extensions to FD.  The string table is shifted by
>> +   STRING_TABLE_OFFSET.  The extension directory is assumed to be
>> +   located at CACHE_EXTENSION_OFFSET.  assign_glibc_hwcaps_indices
>> +   must have been called.  */
>>  static void
>> -write_extensions (int fd, uint32_t cache_extension_offset)
>> +write_extensions (int fd, uint32_t str_offset,
>> +		  uint32_t cache_extension_offset)
>>  {
>>    assert ((cache_extension_offset % 4) == 0);
>>  
>> +  /* The length and contents of the glibc-hwcaps section.  */
>> +  uint32_t hwcaps_count = glibc_hwcaps_count ();
>> +  uint32_t hwcaps_offset = cache_extension_offset + cache_extension_size;
>> +  uint32_t hwcaps_size = hwcaps_count * sizeof (uint32_t);
>> +  uint32_t *hwcaps_array = xmalloc (hwcaps_size);
>> +  for (struct glibc_hwcaps_subdirectory *p = hwcaps; p != NULL; p = p->next)
>> +    if (p->used)
>> +      hwcaps_array[p->section_index] = str_offset + p->name->offset;
>> +
>> +  /* This is the offset of the generator string.  */
>> +  uint32_t generator_offset = hwcaps_offset;
>> +  if (hwcaps_count == 0)
>> +    /* There is no section for the hwcaps subdirectories.  */
>> +    generator_offset -= sizeof (struct cache_extension_section);
>> +  else
>> +    /* The string table indices for the hwcaps subdirectories shift
>> +       the generator string backwards.  */
>> +    generator_offset += hwcaps_count * sizeof (uint32_t);
>
> Maybe 
>
>   generator_offset += hwcaps_size;

Fixed.

>>    struct cache_extension *ext = xmalloc (cache_extension_size);
>>    ext->magic = cache_extension_magic;
>> -  ext->count = cache_extension_count;
>>  
>> -  for (int i = 0; i < cache_extension_count; ++i)
>> -    {
>> -      ext->sections[i].tag = i;
>> -      ext->sections[i].flags = 0;
>> -    }
>
> Ok, although maybe you could refactor the 'elf: Add extension
> mechanism to ld.so.cache' to avoid add such code.

This is still quite ad-hoc.  I expect that the code will change again
when we have additional extensions and a common pattern emerges.

>> +  /* Extension index current being filled.  */
>> +  size_t xid = 0;
>>  
>>    const char *generator
>>      = "ldconfig " PKGVERSION RELEASE " release version " VERSION;
>> -  ext->sections[cache_extension_tag_generator].offset
>> -    = cache_extension_offset + cache_extension_size;
>> -  ext->sections[cache_extension_tag_generator].size = strlen (generator);
>> +  ext->sections[xid].tag = cache_extension_tag_generator;
>> +  ext->sections[xid].flags = 0;
>> +  ext->sections[xid].offset = generator_offset;
>> +  ext->sections[xid].size = strlen (generator);
>> +
>> +  if (hwcaps_count > 0)
>> +    {
>> +      ++xid;
>> +      ext->sections[xid].tag = cache_extension_tag_glibc_hwcaps;
>> +      ext->sections[xid].flags = 0;
>> +      ext->sections[xid].offset = hwcaps_offset;
>> +      ext->sections[xid].size = hwcaps_size;
>> +    }
>> +
>> +  ++xid;
>> +  ext->count = xid;
>> +  assert (xid <= cache_extension_count);
>
> Would it make more sense to reference the index directly using the
> enumeration instead or add an assert to check if the index is within
> the expected size?

In the future, the index does not necessarily equal the tag value.  We
don't write the glibc-hwcaps extension if no such subdirectories exist.

>> -  if (write (fd, ext, cache_extension_size) != cache_extension_size
>> +  size_t ext_size = (offsetof (struct cache_extension, sections)
>> +		     + xid * sizeof (struct cache_extension_section));
>
> So here we could just use cache_extension_count instead of 'xid' (with
> the advantage that we certify at compile-time that only know
> cache_extension_count will be written on the file).

I think we shouldn't write extensions that aren't used.  It will help to
make sure that the loader code is tolerant of extensions.

>> @@ -838,7 +932,10 @@ search_dir (const struct dir_entry *entry)
>>        else
>>  	is_dir = S_ISDIR (lstat_buf.st_mode);
>>  
>> -      if (is_dir && is_hwcap_platform (direntry->d_name))
>> +      /* No descending into subdirectories if this directory is a
>> +	 glibc-hwcaps subdirectory (which are not recursive).  */
>> +      if (entry->hwcaps == NULL
>> +	  && is_dir && is_hwcap_platform (direntry->d_name))
>>  	{
>>  	  if (!is_link
>>  	      && direntry->d_type != DT_UNKNOWN
>
> is_dir is an 'int', maybe make it a boolean?

Fixed.

>> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
>> index fec209509d..66b0312ac1 100644
>> --- a/sysdeps/generic/dl-cache.h
>> +++ b/sysdeps/generic/dl-cache.h
>> @@ -81,7 +81,6 @@ struct cache_file
>>  #define CACHE_VERSION "1.1"
>>  #define CACHEMAGIC_VERSION_NEW CACHEMAGIC_NEW CACHE_VERSION
>>  
>> -
>
> Spurious line removal.

Fixed.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill