| Message ID | 20240209125449.2352780-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | elf: Properly remove the initial 'env' command | expand |
* H. J. Lu: > index 9e70e74bf8..dfba94de64 100644 > --- a/elf/tst-rtld-list-diagnostics.py > +++ b/elf/tst-rtld-list-diagnostics.py > @@ -294,7 +294,11 @@ def main(argv): > check_consistency_with_manual(opts.manual) > > # Remove the initial 'env' command. > - parse_diagnostics(opts.command.split()[1:]) > + options = [] > + for o in opts.command.split()[0:]: > + if o != 'env': > + options.append(o) > + parse_diagnostics(options) I think you can write this as: options = opts.command.split()[:] options.remove('env') It only removes the first occurrence, but I think that is more correct anyway. Thanks, Florian
On Feb 09 2024, H.J. Lu wrote: > diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py > index 9e70e74bf8..dfba94de64 100644 > --- a/elf/tst-rtld-list-diagnostics.py > +++ b/elf/tst-rtld-list-diagnostics.py > @@ -294,7 +294,11 @@ def main(argv): > check_consistency_with_manual(opts.manual) > > # Remove the initial 'env' command. > - parse_diagnostics(opts.command.split()[1:]) > + options = [] > + for o in opts.command.split()[0:]: > + if o != 'env': > + options.append(o) Why does it need to do that in the first place?
On Fri, Feb 9, 2024 at 5:00 AM Florian Weimer <fweimer@redhat.com> wrote: > > * H. J. Lu: > > > index 9e70e74bf8..dfba94de64 100644 > > --- a/elf/tst-rtld-list-diagnostics.py > > +++ b/elf/tst-rtld-list-diagnostics.py > > @@ -294,7 +294,11 @@ def main(argv): > > check_consistency_with_manual(opts.manual) > > > > # Remove the initial 'env' command. > > - parse_diagnostics(opts.command.split()[1:]) > > + options = [] > > + for o in opts.command.split()[0:]: > > + if o != 'env': > > + options.append(o) > > + parse_diagnostics(options) > > I think you can write this as: > > options = opts.command.split()[:] > options.remove('env') > > It only removes the first occurrence, but I think that is more correct > anyway. > Fixed in v2. Thanks.
* Andreas Schwab: > On Feb 09 2024, H.J. Lu wrote: > >> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py >> index 9e70e74bf8..dfba94de64 100644 >> --- a/elf/tst-rtld-list-diagnostics.py >> +++ b/elf/tst-rtld-list-diagnostics.py >> @@ -294,7 +294,11 @@ def main(argv): >> check_consistency_with_manual(opts.manual) >> >> # Remove the initial 'env' command. >> - parse_diagnostics(opts.command.split()[1:]) >> + options = [] >> + for o in opts.command.split()[0:]: >> + if o != 'env': >> + options.append(o) > > Why does it need to do that in the first place? Good question. I must have copied it from scripts/tst-ld-trace.py. I think we can remove the .split() and run the command string with shell=True. Thanks, Florian
On Fri, Feb 9, 2024 at 5:30 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Andreas Schwab: > > > On Feb 09 2024, H.J. Lu wrote: > > > >> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py > >> index 9e70e74bf8..dfba94de64 100644 > >> --- a/elf/tst-rtld-list-diagnostics.py > >> +++ b/elf/tst-rtld-list-diagnostics.py > >> @@ -294,7 +294,11 @@ def main(argv): > >> check_consistency_with_manual(opts.manual) > >> > >> # Remove the initial 'env' command. > >> - parse_diagnostics(opts.command.split()[1:]) > >> + options = [] > >> + for o in opts.command.split()[0:]: > >> + if o != 'env': > >> + options.append(o) > > > > Why does it need to do that in the first place? > > Good question. I must have copied it from scripts/tst-ld-trace.py. I > think we can remove the .split() and run the command string with How does removing .split() work? The argument is "/export/build/gnu/tools-build/glibc-apx-sde/test-sde.sh env /export/build/gnu/tools-build/glibc-apx-sde/build-x86_64-linux/elf/ld-linux-x86-64.so.2 --list-diagnostics" We need .split() to change it to proper arguments. > shell=True. > > Thanks, > Florian >
* H. J. Lu: > On Fri, Feb 9, 2024 at 5:30 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> * Andreas Schwab: >> >> > On Feb 09 2024, H.J. Lu wrote: >> > >> >> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py >> >> index 9e70e74bf8..dfba94de64 100644 >> >> --- a/elf/tst-rtld-list-diagnostics.py >> >> +++ b/elf/tst-rtld-list-diagnostics.py >> >> @@ -294,7 +294,11 @@ def main(argv): >> >> check_consistency_with_manual(opts.manual) >> >> >> >> # Remove the initial 'env' command. >> >> - parse_diagnostics(opts.command.split()[1:]) >> >> + options = [] >> >> + for o in opts.command.split()[0:]: >> >> + if o != 'env': >> >> + options.append(o) >> > >> > Why does it need to do that in the first place? >> >> Good question. I must have copied it from scripts/tst-ld-trace.py. I >> think we can remove the .split() and run the command string with > > How does removing .split() work? The argument is > > "/export/build/gnu/tools-build/glibc-apx-sde/test-sde.sh env > /export/build/gnu/tools-build/glibc-apx-sde/build-x86_64-linux/elf/ld-linux-x86-64.so.2 > --list-diagnostics" > > We need .split() to change it to proper arguments. Can you test this? diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py index 9e70e74bf8..024bd8c320 100644 --- a/elf/tst-rtld-list-diagnostics.py +++ b/elf/tst-rtld-list-diagnostics.py @@ -222,7 +222,7 @@ else: def parse_diagnostics(cmd): global errors diag_out = subprocess.run(cmd, stdout=subprocess.PIPE, check=True, - universal_newlines=True).stdout + universal_newlines=True, shell=True).stdout if diag_out[-1] != '\n': print('error: ld.so output does not end in newline') errors += 1 @@ -293,8 +293,7 @@ def main(argv): if opts.manual: check_consistency_with_manual(opts.manual) - # Remove the initial 'env' command. - parse_diagnostics(opts.command.split()[1:]) + parse_diagnostics(opts.command) if errors: sys.exit(1) Thanks, Florian
On Fri, Feb 9, 2024 at 6:53 AM Florian Weimer <fweimer@redhat.com> wrote: > > * H. J. Lu: > > > On Fri, Feb 9, 2024 at 5:30 AM Florian Weimer <fweimer@redhat.com> wrote: > >> > >> * Andreas Schwab: > >> > >> > On Feb 09 2024, H.J. Lu wrote: > >> > > >> >> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py > >> >> index 9e70e74bf8..dfba94de64 100644 > >> >> --- a/elf/tst-rtld-list-diagnostics.py > >> >> +++ b/elf/tst-rtld-list-diagnostics.py > >> >> @@ -294,7 +294,11 @@ def main(argv): > >> >> check_consistency_with_manual(opts.manual) > >> >> > >> >> # Remove the initial 'env' command. > >> >> - parse_diagnostics(opts.command.split()[1:]) > >> >> + options = [] > >> >> + for o in opts.command.split()[0:]: > >> >> + if o != 'env': > >> >> + options.append(o) > >> > > >> > Why does it need to do that in the first place? > >> > >> Good question. I must have copied it from scripts/tst-ld-trace.py. I > >> think we can remove the .split() and run the command string with > > > > How does removing .split() work? The argument is > > > > "/export/build/gnu/tools-build/glibc-apx-sde/test-sde.sh env > > /export/build/gnu/tools-build/glibc-apx-sde/build-x86_64-linux/elf/ld-linux-x86-64.so.2 > > --list-diagnostics" > > > > We need .split() to change it to proper arguments. > > Can you test this? > > diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py > index 9e70e74bf8..024bd8c320 100644 > --- a/elf/tst-rtld-list-diagnostics.py > +++ b/elf/tst-rtld-list-diagnostics.py > @@ -222,7 +222,7 @@ else: > def parse_diagnostics(cmd): > global errors > diag_out = subprocess.run(cmd, stdout=subprocess.PIPE, check=True, > - universal_newlines=True).stdout > + universal_newlines=True, shell=True).stdout > if diag_out[-1] != '\n': > print('error: ld.so output does not end in newline') > errors += 1 > @@ -293,8 +293,7 @@ def main(argv): > if opts.manual: > check_consistency_with_manual(opts.manual) > > - # Remove the initial 'env' command. > - parse_diagnostics(opts.command.split()[1:]) > + parse_diagnostics(opts.command) > > if errors: > sys.exit(1) > > Thanks, > Florian > It works. Can you check it in? Thanks.
* H. J. Lu:
> It works. Can you check it in?
Thanks for checking, pushed.
Florian
diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py index 9e70e74bf8..dfba94de64 100644 --- a/elf/tst-rtld-list-diagnostics.py +++ b/elf/tst-rtld-list-diagnostics.py @@ -294,7 +294,11 @@ def main(argv): check_consistency_with_manual(opts.manual) # Remove the initial 'env' command. - parse_diagnostics(opts.command.split()[1:]) + options = [] + for o in opts.command.split()[0:]: + if o != 'env': + options.append(o) + parse_diagnostics(options) if errors: sys.exit(1)
tst-rtld-list-diagnostics.py is called by "$(test-wrapper-env) $(objpfx)$(rtld-installed-name) --list-diagnostics" and $(test-wrapper-env) is set to "$(test-wrapper) env". When there is a test wrapper, it is incorrect to use: # Remove the initial 'env' command. parse_diagnostics(opts.command.split()[1:]) to remove 'env'. Change tst-rtld-list-diagnostics.py to explicitly check 'env' instead. This fixes [BZ #31357]. --- elf/tst-rtld-list-diagnostics.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)