Re: [PATCH] elf: Fix elf/tst-pldd with --enable-hardcoded-path-in-tests (BZ#24506)

[Carlos O'Donell <codonell at redhat dot com>]

On 5/1/19 1:41 PM, Adhemerval Zanella wrote:
> On 30/04/2019 18:05, Carlos O'Donell wrote:
> > On 4/30/19 3:18 PM, Adhemerval Zanella wrote:
> > > Although test-container does support --enable-hardcoded-path-in-tests,
> > > the resulting library names obtained with pldd will have the 'vanilla'
> > > names of 'ld.so' and 'libc.so' instead of the installed ABI names
> > > (ld-linux-x86-64.so.2 for instance on x86_64).  The patch adds the
> > > default names as one possible option.
> > >
> > > Checked on x86_64-linux-gnu (built with and without
> > > --enable-hardcoded-path-in-tests) and i686-linux-gnu.
> >
> > What does this patch do?
> >
> > Please explain in detail the problem and why this is a solution :-)

Overall the patch looks good to me.

OK for master if you cleanup your commit message to be a meaninful and
self-contained description of the problem, and the fix.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> Sorry if I was not clean in first paragraph.  The patch adds the canonical
> names for loader and libc as one option while checking the pldd output.
> It is required because when --enable-hardcoded-path-in-tests, the tests
> are linked against ld.so and libc.so instead of the installed names
> (ld-linux-x86-64.so.2 for instance on x86_64).

You still haven't explained what is broken, and that's required for good
self-contained commit message.

> What about the following line in commit message:
>
> --
> Although test-container does support --enable-hardcoded-path-in-tests,
> the resulting library names obtained with pldd will have the 'vanilla'
> names of 'ld.so' and 'libc.so' instead of the installed ABI names
> (ld-linux-x86-64.so.2 for instance on x86_64).  The patch adds them
> as one option while checking the pldd output.
> ---

When you make a commit message think C-C-F-R.

Cause. Consequence. Fix. Result.

You will write spectacular notes if you keep CCFR in mind.

Consider:
~~~
The elf/tst-pldd test does not expect the hardcoded paths that are
output by pldd when the test is built with
--enable-hardcoded-path-in-tests.

The elf/tst-pldd test fails when glibc is compiled with the developer
option --enable-hardcoded-path-in-tests.

We enhance elf/tst-pldd to correctly process the harcoded paths.

With this enhancement the elf/tst-pldd test no longer fails when glibc
is compiled with --enable-harcoded-path-in-tests
~~~

See how each sentence ties into CCFR.

Start with CCFR and then add any additional notes you want and you
won't have any problems with reviewers.

You can always rewrite it so the sentences flow together better and
don't duplicate information, but if you start with CCFR you'll be
starting off with a quality commit message.

You may understand the problem, but that's because you're focused on
it entirely, while the reviewer has only a slice of time to review.

> > >      * elf/tst-pldd.c (in_str_list): New function.
> > >      (do_test): Add default names for ld and libc as one option.
> > > ---
> > >    elf/tst-pldd.c | 20 +++++++++++++++++---
> > >    1 file changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
> > > index ed19cedd05..2a9f58936f 100644
> > > --- a/elf/tst-pldd.c
> > > +++ b/elf/tst-pldd.c
> > > @@ -20,7 +20,6 @@
> > >    #include <string.h>
> > >    #include <unistd.h>
> > >    #include <stdint.h>
> > > -#include <libgen.h>

OK.

> > >    #include <stdbool.h>
> > >      #include <array_length.h>
> > > @@ -39,6 +38,15 @@ target_process (void *arg)
> > >    /* The test runs in a container because pldd does not support tracing
> > >       a binary started by the loader iself (as with testrun.sh).  */
> > >    +static bool
> > > +in_str_list (const char *libname, const char *const strlist[])
> > > +{
> > > +  for (const char *const *str = strlist; *str != NULL; str++)
> > > +    if (strcmp (libname, *str) == 0)
> > > +      return true;
> > > +  return false;
> > > +}

OK.

> > > +
> > >    static int
> > >    do_test (void)
> > >    {
> > > @@ -89,14 +97,20 @@ do_test (void)
> > >        if (buffer[strlen(buffer)-1] == '\n')
> > >          buffer[strlen(buffer)-1] = '\0';
> > >    -    if (strcmp (basename (buffer), LD_SO) == 0)
> > > +    const char *libname = basename (buffer);
> > > +
> > > +    /* It checks for default names in case of build configure with
> > > +       --enable-hardcoded-path-in-tests (BZ #24506).  */
> > > +    if (in_str_list (libname,
> > > +             (const char *const []) { "ld.so", LD_SO, NULL }))

OK.

> > >          {
> > >            TEST_COMPARE (interpreter_found, false);
> > >            interpreter_found = true;
> > >            continue;
> > >          }
> > >    -    if (strcmp (basename (buffer), LIBC_SO) == 0)
> > > +    if (in_str_list (libname,
> > > +             (const char *const []) { "libc.so", LIBC_SO, NULL }))

OK.

> > >          {
> > >            TEST_COMPARE (libc_found, false);
> > >            libc_found = true;


--
Cheers,
Carlos.