common/funcs/tst.basename.d*: use env to find ksh in $PATH

[ngie]

Summary

tst.basename.d hardcoded one of the solaris paths to ksh instead of using env to find it.

This change helps ensure that the script that gets run by tst.basename.d is found dynamically and it gets interpreted by dtest.pl properly.

Test Plan

$ sudo kyua test -k /usr/tests/cddl/usr.sbin/dtrace/common/funcs/Kyuafile t_dtrace_contrib:tst_basename_d
t_dtrace_contrib:tst_basename_d  ->  passed  [1.420s]

Results file id is usr_tests_cddl_usr.sbin_dtrace_common_funcs.20170723-180611-402683
Results saved to /root/.kyua/store/results.usr_tests_cddl_usr.sbin_dtrace_common_funcs.20170723-180611-402683.db

1/1 passed (0 failed)
$ sudo kyua test -k /usr/tests/cddl/usr.sbin/dtrace/common/safety/Kyuafile t_dtrace_contrib:tst_basename_d                                                                           
t_dtrace_contrib:tst_basename_d  ->  passed  [14.925s]

Results file id is usr_tests_cddl_usr.sbin_dtrace_common_safety.20170723-180634-128449
Results saved to /root/.kyua/store/results.usr_tests_cddl_usr.sbin_dtrace_common_safety.20170723-180634-128449.db

1/1 passed (0 failed)

[ngie]

This is the wrong solution -- markj might have a different one that's in OneFS that hasn't been upstreamed yet.

/usr/bin/ksh should be replaced with /usr/bin/env ksh, or another equivalent interpreter path:

$ grep -r '/usr/bin/ksh' cddl/
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/funcs/tst.basename.d:       printf("#!/usr/bin/ksh\n\n");
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/funcs/tst.basename.d.out:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localtcp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localudp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4remoteicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4remotetcp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4remoteudp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv6localicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv6remoteicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.localtcpstate.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.remotetcpstate.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/err.invalidtype2.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/err.invalidtype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.chasestrings.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/err.user64mode.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.aouttype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.libtype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.linkmap.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.pidprint.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.pidprinttarg.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.printtype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.printtypetarg.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.userlandkey.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.userstrings.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/usdt/tst.forker.ksh:#!/usr/bin/ksh
$

[lwhsu]

• Add shebang to pet lint
• s/tst.basename.d.out/$STDOUT/ that was for testing and forgot to change before submitting

[lwhsu]

In D11675#242002, @ngie wrote:

This is the wrong solution -- markj might have a different one that's in OneFS that hasn't been upstreamed yet.

@markj : could you share your approach to this issue?

/usr/bin/ksh should be replaced with /usr/bin/env ksh, or another equivalent interpreter path:

This is the test output, which is a verifying script which should be run. What I do here is checking if the output is a script and run it then check its return value.

$ grep -r '/usr/bin/ksh' cddl/
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/funcs/tst.basename.d:       printf("#!/usr/bin/ksh\n\n");

This is why the output script beginning with #!/usr/bin/ksh

cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/funcs/tst.basename.d.out:#!/usr/bin/ksh

And this is .out file, which is the dtest script compares output of tst.basename.d with.

cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localtcp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4localudp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4remoteicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4remotetcp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv4remoteudp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv6localicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.ipv6remoteicmp.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.localtcpstate.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/ip/tst.remotetcpstate.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/err.invalidtype2.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/err.invalidtype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.chasestrings.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/err.user64mode.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.aouttype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.libtype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.linkmap.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.pidprint.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.pidprinttarg.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.printtype.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.printtypetarg.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.userlandkey.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/uctf/tst.userstrings.ksh:#! /usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh:#!/usr/bin/ksh
cddl/contrib/opensolaris/cmd/dtrace/test/tst/common/usdt/tst.forker.ksh:#!/usr/bin/ksh
$

Those are test scripts, which will be run by dtest directly, and compare the output with *.ksh.out, if exists.

[ngie]

Please don't commit this change. I have an alternate change I'll be submitting soon that will fix the root cause.

[ngie]

Please don't commit this change. I have an alternate change I'll be submitting soon that will fix the root cause.

cddl/usr.sbin/dtrace/tests/tools/dtest.sh

1 ↗

(On Diff #31052)

This is wrong. Phabricator is correctly complaining about this, but the script should be chmod -x since atf.test.mk will spams shebang with "make all". I'll fix this.

[lwhsu]

Remove shebang line, that will be done by others.

[lwhsu]

In D11675#242121, @ngie wrote:

Please don't commit this change. I have an alternate change I'll be submitting soon that will fix the root cause.

ok, just wanted to keep a note that tst.basename.d case is not run correctly.

[markj]

In D11675#242002, @ngie wrote:

This is the wrong solution -- markj might have a different one that's in OneFS that hasn't been upstreamed yet.

/usr/bin/ksh should be replaced with /usr/bin/env ksh, or another equivalent interpreter path:

That's a separate issue, and not really a problem to begin with. Both dtest.sh and upstream's test harness invoke ksh scripts with "<path to ksh> <script>" instead of making the script executable. So the shebang lines are effectively unused.

In D11675#242113, @lwhsu wrote:

In D11675#242002, @ngie wrote:

This is the wrong solution -- markj might have a different one that's in OneFS that hasn't been upstreamed yet.

@markj : could you share your approach to this issue?

I don't have any changes that are relevant here.

I'm surprised by the tst.basename.d test - it happens to pass currently. How did you find it? I also note that it doesn't pass because the second-last test is not quoted properly.

cddl/usr.sbin/dtrace/tests/tools/dtest.sh

89 ↗

(On Diff #31055)

This should use an anchor, and perhaps avoid hard-coding the path. Perhaps '^#!/.*ksh$' instead?

[lwhsu]

• Add back shebang line, I checked there is no shebang in /usr/tests/cddl/usr.sbin/dtrace/dtest and this is what happens while install: install -o root -g wheel -m 555 /usr/src/cddl/usr.sbin/dtrace/tests/tools/dtest.sh /usr/tests/cddl/usr.sbin/dtrace/dtest

• Use stricter regexp and add '-E' to explicitly state the pattern is a regexp

[lwhsu]

cddl/usr.sbin/dtrace/tests/tools/dtest.sh

1 ↗

(On Diff #31052)

I checked the installation process and /usr/tests/cdl/user.sbin/dtrace/dtest it turns out we still need this.

[lwhsu]

In D11675#242169, @markj wrote:

I'm surprised by the tst.basename.d test - it happens to pass currently. How did you find it? I also note that it doesn't pass because the second-last test is not quoted properly.

It constantly timeout: https://ci.freebsd.org/job/FreeBSD-head-amd64-dtrace_test/134/testReport/common.funcs/t_dtrace_contrib/tst_basename_d/
and I have D11676 for dealing with it. While examining the root cause, I found Note that the output of this is a ksh script. When run, it will give no output if the output is correct. in comment.

[ngie]

Use env(1) to find ksh in $PATH instead of using incorrect hardcoded path to it

[markj]

In D11675#242492, @ngie wrote:

Use env(1) to find ksh in $PATH instead of using incorrect hardcoded path to it

???

The previous revision was attempting to address a real problem. I was waiting for Li-Wen to address my observation that the last two test cases are broken: apparently the basename/dirname of the empty string is "." on Solaris, but it's the empty string on FreeBSD. Now, the revision addresses an unrelated cosmetic issue, and the change is simple enough that it does not require review.

[lwhsu]

@ngie, this patch solves another issue, both of our patches are needed.

Please read cddl/usr.sbin/dtrace/tests/tools/dtest.sh, tst.basename.d file will be matched in line 31, and processed by dtrace in line 54, script output will be save in $STDOUT, which is a temporary file, whose name is generated in line 93.

Then, $STDOUT will be compared with $EXOUT (tst.basename.d.out) line 102. If the contents in both files are identical, then test passed. That's how tst.*.d and tst.*.d.out test cases work.

But this is NOT enough for the tst.basename.d and tst.basename.d.out case, whose output is a script, and need to be executed and check if there is no output, that's all the complete test steps.

What I am doing is adding the final step: executing the generated script and check the result. In my patch it is done by ksh $STDOUT, so the shebang line is not that important, while it is also good to change that, because that helps for executing it independently, which will help debug.

Please resume my old patch (and added yours if you want) and let more people review.

[lwhsu]

In D11675#242520, @markj wrote:

I was waiting for Li-Wen to address my observation that the last two test cases are broken: apparently the basename/dirname of the empty string is "." on Solaris, but it's the empty string on FreeBSD.

Sorry that I did not know you're waiting for that, do you think we should modify the test case, make it accept both "" and ".", or modify DTrace code (unlikely)?

[lwhsu]

In D11675#242527, @lwhsu wrote:

Please resume my old patch (and added yours if you want) and let more people review.

@ngie, if you cannot restore the author, I am happy to re-commander this patch.

[markj]

In D11675#242528, @lwhsu wrote:

In D11675#242520, @markj wrote:

I was waiting for Li-Wen to address my observation that the last two test cases are broken: apparently the basename/dirname of the empty string is "." on Solaris, but it's the empty string on FreeBSD.

Sorry that I did not know you're waiting for that, do you think we should modify the test case, make it accept both "" and ".", or modify DTrace code (unlikelay)?

Hmm. Looking more closely, I think this is actually a bug in basename(1). The basename(3) man page actually says that the basename of "" is ".", i.e., what Solaris does. But I get:

$ basename ""

$ dirname ""
.
$

In particular, the last test case is fine, it's just the second-last one that's problematic. Looking at usr.bin/basename/basename.c, I see that we just print a newline and exit if *argv[0] == 0. This comes from r67024.

For now I would update the test to accept either "" or ".", and add a comment explaining why.

[lwhsu]

In D11675#242530, @markj wrote:

Hmm. Looking more closely, I think this is actually a bug in basename(1). The basename(3) man page actually says that the basename of "" is ".", i.e., what Solaris does. But I get:

$ basename ""

$ dirname ""
.
$

In particular, the last test case is fine, it's just the second-last one that's problematic. Looking at usr.bin/basename/basename.c, I see that we just print a newline and exit if *argv[0] == 0. This comes from r67024.

For now I would update the test to accept either "" or ".", and add a comment explaining why.

It seems like a feature now, I checked the Linux implementation, basename(1) also returns "" while baseame(3) returns ".".

Anyway, I created D11707 for that issue (and included @ngie's change, this differential should focus on the "executing verifying script" part.

[ngie]

Abandoning revision superseded by D11707.

[lwhsu]

In D11675#242686, @ngie wrote:

Abandoning revision superseded by D11707.

Sigh, the old patch is not needed to be abandoned, but I cannot re-open and re-commander this one so I created D11716.

@ngie, you are welcomed to review that differential, but please read the discussion in this one again carefully, both the test case and driver have bugs. D11707 is fixing the test case and D11716 is fixing the driver.