[PATCH 27/28] elf: Process glibc-hwcaps subdirectories in ldconfig
Florian Weimer
fweimer@redhat.com
Wed Nov 4 11:57:20 GMT 2020
* 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
More information about the Libc-alpha
mailing list