Re: [PATCH v10 1/4] elf: Add la_activity during application exit

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> On 18/01/2022 08:29, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
>>> index de8eb1b3c9..2705a15c88 100644
>>> --- a/elf/dl-fini.c
>>> +++ b/elf/dl-fini.c
>>> @@ -64,6 +64,11 @@ _dl_fini (void)
>>>  	__rtld_lock_unlock_recursive (GL(dl_load_lock));
>>>        else
>>>  	{
>>> +#ifdef SHARED
>>> +	  /* Auditing checkpoint: we will start deleting objects.  */
>>> +	  _dl_audit_activity_nsid (ns, LA_ACT_DELETE);
>>> +#endif
>>> +
>>>  	  /* Now we can allocate an array to hold all the pointers and
>>>  	     copy the pointers in.  */
>>>  	  struct link_map *maps[nloaded];
>>> @@ -153,6 +158,11 @@ _dl_fini (void)
>>>  	      /* Correct the previous increment.  */
>>>  	      --l->l_direct_opencount;
>>>  	    }
>>> +
>>> +#ifdef SHARED
>>> +	  /* Auditing checkpoint: we will start deleting objects.  */
>>> +	  _dl_audit_activity_nsid (ns, LA_ACT_CONSISTENT);
>>> +#endif
>>>  	}
>>>      }
>> 
>> The tense in the second comment seems wrong.
>
> Ack, in fact I think both comments do not add much (it is pretty obvious
> from the function call).  I will remove them.
>
>> 
>>>  
>>> diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
>>> new file mode 100644
>>> index 0000000000..9a7e69c1e1
>>> --- /dev/null
>>> +++ b/elf/tst-audit23.c
>> 
>>> +  FILE *out = fmemopen (result.err.buffer, result.err.length, "r");
>>> +  TEST_VERIFY (out != NULL);
>>> +  char *buffer = NULL;
>>> +  size_t buffer_length = 0;
>>> +  while (xgetline (&buffer, &buffer_length, out))
>>> +    {
>>> +      if (startswith (buffer, "la_activity: "))
>> 
>> It may be simpler (or easier to maintain going forward) to have the
>> expected output (without varying pointers) and compare against that
>> verbatim.  This is what I implemented for some of the DNS tests.
>
> The main issue is to handle the vDSO (is_vdso) and match the cookies
> and address with the subsequent la_activities/la_objclose.  So it
> would require to keep track with some internal state anyway.
>
> And checking on resolve/tst-ns_name.c I don't think it would be
> simpler to use such scheme (specially the required handling it would 
> require to parse a possible input to handle line with multiple options
> and different outputs).

Okay, I just found the existing test rather difficult to review.

Thanks,
Florian