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