[PATCH 06/11] elf: Implement a string table for ldconfig, with tail merging

[Adhemerval Zanella]

On 27/11/2020 16:49, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> +static void
>>> +stringtable_init (struct stringtable *table)
>>> +{
>>> +  table->count = 0;
>>> +
>>> +  /* This needs to be a power of two.  128 is sufficient to keep track
>>> +     of 42 DSOs without resizing (assuming two strings per DSOs).
>>> +     glibc itself comes with more than 20 DSOs, so 64 would likely to
>>> +     be too small.  */
>>> +  table->allocated = 64;
>>
>> Should we use a large value then? Asking because the comment indicates that
>> the chosen default value is not best suitable one.
> 
> You mean 128?  Eh, yes.
> 
>>> +/* Double the capacity of the hash table.  */
>>> +static void
>>> +stringtable_rehash (struct stringtable *table)
>>> +{
>>> +  /* Cannot overflow because the old allocation size (in bytes) is
>>> +     larger.  */
>>
>> This comment is a bit confusing, I think it should be safe to assume
>> it won't overflow because stringtable_rehash won't be called for
>> large values (stringtable_add will bail early).
> 
> What I meant is this:
> 
>   /* This computation cannot overflow because the old total in-memory
>      size of the hash table is larger than the computed value.  */

Ack.

> 
>>> +void
>>> +stringtable_finalize (struct stringtable *table,
>>> +                      struct stringtable_finalized *result)
>>> +{
>>> +  if (table->count == 0)
>>> +    {
>>> +      result->strings = xstrdup ("");
>>> +      result->size = 0;
>>> +      return;
>>> +    }
>>
>> This is confusing because the folliwing.
>>
>>   struct stringtable s = { 0, };
>>   struct stringtable_finalized f;
>>   stringtable_finalize (&s, &f);
>>   
>> will result in f being { "", 0 }, while the following
>>
>>   struct stringtable s = { 0, };
>>   stringtable_add (&s, "");
>>   struct stringtable_finalized f;
>>   stringtable_finalize (&s, &f);
>>   
>> will result in f being { "", 1 }.
>>
>> I would expect that if strings is being "", the 'size' should be the
>> same (either 0 or 1 for both usages).
> 
> In the second space, storage space is needed for the null byte because
> the table contains the empty string.  In the first case, no storage
> space is needed.  We could use NULL, but then glibc has a bit of a
> strange relationship with arrays of length 0 that start at a null
> pointer.  (They can't be used in memcpy etc.)

Fair enough. 

> 
> Thanks,
> Florian
>