diff mbox series

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

Message ID 20190430191834.19003-1-adhemerval.zanella@linaro.org
State Accepted
Commit b2af6fb2ed23930c148bae382ca85fad4d1cf32e
Headers show
Series elf: Fix elf/tst-pldd with --enable-hardcoded-path-in-tests (BZ#24506) | expand

Commit Message

Adhemerval Zanella April 30, 2019, 7:18 p.m. UTC
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.

	* 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(-)

-- 
2.17.1

Comments

Carlos O'Donell April 30, 2019, 9:05 p.m. UTC | #1
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 :-)

> 	* 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>

>   #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;

> +}

> +

>   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 }))

>   	  {

>   	    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 }))

>   	  {

>   	    TEST_COMPARE (libc_found, false);

>   	    libc_found = true;

> 



-- 
Cheers,
Carlos.
Adhemerval Zanella May 1, 2019, 5:41 p.m. UTC | #2
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 :-)


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).

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.
---

> 

>>     * 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>

>>   #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;

>> +}

>> +

>>   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 }))

>>         {

>>           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 }))

>>         {

>>           TEST_COMPARE (libc_found, false);

>>           libc_found = true;

>>

> 

>
Carlos O'Donell May 1, 2019, 6:12 p.m. UTC | #3
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.
Adhemerval Zanella May 1, 2019, 6:36 p.m. UTC | #4
On 01/05/2019 15:12, Carlos O'Donell wrote:
> 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.


Fair enough, I followed your suggestion and changes the commit message to:

--
    The elf/tst-pldd (added by 1a4c27355e146 to fix BZ#18035) test does
    not expect the hardcoded paths that are output by pldd when the test
    is built with --enable-hardcoded-path-in-tests.  Instead of showing
    the ABI installed library names for loader and libc (such as
    ld-linux-x86-64.so.2 and libc.so.6 for x86_64), pldd shows the default
    built ld.so and libc.so.
    
    It makes the tests fail with an invalid expected loader/libc name.
    
    This patch fixes the elf-pldd test by adding the canonical ld.so and
    libc.so names in the expected list of possible outputs when parsing
    the result output from pldd.  The test now handles both default
    build and --enable-hardcoded-path-in-tests option.
    
    Checked on x86_64-linux-gnu (built with and without
    --enable-hardcoded-path-in-tests) and i686-linux-gnu.
--


> 

>>>

>>>>      * 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;

>>>>

>>>

>>>

> 

>
Carlos O'Donell May 1, 2019, 9:11 p.m. UTC | #5
On 5/1/19 2:36 PM, Adhemerval Zanella wrote:
> On 01/05/2019 15:12, Carlos O'Donell wrote:

>> 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.

> 

> Fair enough, I followed your suggestion and changes the commit message to:

> 

> --

>      The elf/tst-pldd (added by 1a4c27355e146 to fix BZ#18035) test does

>      not expect the hardcoded paths that are output by pldd when the test

>      is built with --enable-hardcoded-path-in-tests.  Instead of showing

>      the ABI installed library names for loader and libc (such as

>      ld-linux-x86-64.so.2 and libc.so.6 for x86_64), pldd shows the default

>      built ld.so and libc.so.

>      

>      It makes the tests fail with an invalid expected loader/libc name.

>      

>      This patch fixes the elf-pldd test by adding the canonical ld.so and

>      libc.so names in the expected list of possible outputs when parsing

>      the result output from pldd.  The test now handles both default

>      build and --enable-hardcoded-path-in-tests option.

>      

>      Checked on x86_64-linux-gnu (built with and without

>      --enable-hardcoded-path-in-tests) and i686-linux-gnu.


Spectacular commit message!

-- 
Cheers,
Carlos.
diff mbox series

Patch

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>
 #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;
+}
+
 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 }))
 	  {
 	    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 }))
 	  {
 	    TEST_COMPARE (libc_found, false);
 	    libc_found = true;