| Message ID | 20220428065657.32046-1-pvorel@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi, TST_TESTFUNC should be moved to individual test scripts as well, for the same reason. Other than that, the whole patchset looks good: Reviewed-by: Martin Doucha <mdoucha@suse.cz> On 28. 04. 22 8:56, Petr Vorel wrote: > Although having variables in both busy_poll_lib.sh and the tests which > are using it isn't optimal, hooking up callbacks on the reverse end of > include is even worse code. > > Suggested-by: Martin Doucha <mdoucha@suse.cz> > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Follow-up of "v3 shell: Cleanup getopts usage" patchset [1], > replacing first commit. > > Kind regards, > Petr > > [1] https://patchwork.ozlabs.org/project/ltp/list/?series=297175 > > testcases/network/busy_poll/busy_poll01.sh | 3 +++ > testcases/network/busy_poll/busy_poll02.sh | 3 +++ > testcases/network/busy_poll/busy_poll03.sh | 2 ++ > testcases/network/busy_poll/busy_poll_lib.sh | 3 +-- > 4 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/testcases/network/busy_poll/busy_poll01.sh b/testcases/network/busy_poll/busy_poll01.sh > index 65f4db3fe..1f7097771 100755 > --- a/testcases/network/busy_poll/busy_poll01.sh > +++ b/testcases/network/busy_poll/busy_poll01.sh > @@ -4,6 +4,9 @@ > # > # Author: Alexey Kodanev <alexey.kodanev@oracle.com> > > +TST_SETUP="setup" > +TST_CLEANUP="cleanup" > + > cleanup() > { > [ -n "$busy_read_old" ] && \ > diff --git a/testcases/network/busy_poll/busy_poll02.sh b/testcases/network/busy_poll/busy_poll02.sh > index ebae4d2f5..634bbd6bd 100755 > --- a/testcases/network/busy_poll/busy_poll02.sh > +++ b/testcases/network/busy_poll/busy_poll02.sh > @@ -4,6 +4,9 @@ > # > # Author: Alexey Kodanev <alexey.kodanev@oracle.com> > > +TST_SETUP="setup" > +TST_CLEANUP="cleanup" > + > cleanup() > { > [ -n "$busy_poll_old" ] && \ > diff --git a/testcases/network/busy_poll/busy_poll03.sh b/testcases/network/busy_poll/busy_poll03.sh > index 04d5978f7..b2e1c0a7a 100755 > --- a/testcases/network/busy_poll/busy_poll03.sh > +++ b/testcases/network/busy_poll/busy_poll03.sh > @@ -4,6 +4,8 @@ > # > # Author: Alexey Kodanev <alexey.kodanev@oracle.com> > > +TST_SETUP="setup" > +TST_CLEANUP="cleanup" > TST_TEST_DATA="udp udp_lite" > > cleanup() > diff --git a/testcases/network/busy_poll/busy_poll_lib.sh b/testcases/network/busy_poll/busy_poll_lib.sh > index de61d3fcd..446ae3d65 100755 > --- a/testcases/network/busy_poll/busy_poll_lib.sh > +++ b/testcases/network/busy_poll/busy_poll_lib.sh > @@ -1,10 +1,9 @@ > #!/bin/sh > # SPDX-License-Identifier: GPL-2.0-or-later > +# Copyright (c) Linux Test Project, 2016-2022 > # Copyright (c) 2016-2018 Oracle and/or its affiliates. All Rights Reserved. > > -TST_SETUP="setup" > TST_TESTFUNC="test" ^^ here > -TST_CLEANUP="cleanup" > TST_MIN_KVER="3.11" > TST_NEEDS_TMPDIR=1 > TST_NEEDS_ROOT=1
Hi Martin, > Hi, > TST_TESTFUNC should be moved to individual test scripts as well, for the > same reason. Other than that, the whole patchset looks good: I was thinking about it as well. Makes sense, but then it hides the fact that busy_poll_lib.sh contain other API variables (missing TST_TESTFUNC suggest that there are some variables in busy_poll_lib.sh). Anyway, I'm ok to move it there as well, just document that sharing variables between library and test will be always a bit problematic. > Reviewed-by: Martin Doucha <mdoucha@suse.cz> Thanks! Kind regards, Petr
On 29. 04. 22 20:09, Petr Vorel wrote: > I was thinking about it as well. Makes sense, but then it hides the fact that > busy_poll_lib.sh contain other API variables (missing TST_TESTFUNC suggest that > there are some variables in busy_poll_lib.sh). Anyway, I'm ok to move it there > as well, just document that sharing variables between library and test will be > always a bit problematic. Now that you mention the other variables in busy_poll_lib.sh, I guess that TST_MIN_KVER and TST_NEEDS_CMDS should use conditional expansion so that the values can be changed in the tests.
Hi Martin, > On 29. 04. 22 20:09, Petr Vorel wrote: > > I was thinking about it as well. Makes sense, but then it hides the fact that > > busy_poll_lib.sh contain other API variables (missing TST_TESTFUNC suggest that > > there are some variables in busy_poll_lib.sh). Anyway, I'm ok to move it there > > as well, just document that sharing variables between library and test will be > > always a bit problematic. > Now that you mention the other variables in busy_poll_lib.sh, I guess > that TST_MIN_KVER and TST_NEEDS_CMDS should use conditional expansion so > that the values can be changed in the tests. Makes sense. But I'd probably prefer to postpone this commit, because TST_NEEDS_CMDS will be in more shell libraries. Because even this cleanup so far wasn't my intention, I'd prefer to get it merged soon so that I can post rebased fixes for tst_net.sh with disabled IPv6 (to get them merged before release). Kind regards, Petr
On 02. 05. 22 15:50, Petr Vorel wrote: > Makes sense. But I'd probably prefer to postpone this commit, > because TST_NEEDS_CMDS will be in more shell libraries. > Because even this cleanup so far wasn't my intention, I'd prefer to get it > merged soon so that I can post rebased fixes for tst_net.sh with disabled IPv6 > (to get them merged before release). Sure, if you plan to fix that in multiple shell libraries, then a separate patchset is a good idea.
diff --git a/testcases/network/busy_poll/busy_poll01.sh b/testcases/network/busy_poll/busy_poll01.sh index 65f4db3fe..1f7097771 100755 --- a/testcases/network/busy_poll/busy_poll01.sh +++ b/testcases/network/busy_poll/busy_poll01.sh @@ -4,6 +4,9 @@ # # Author: Alexey Kodanev <alexey.kodanev@oracle.com> +TST_SETUP="setup" +TST_CLEANUP="cleanup" + cleanup() { [ -n "$busy_read_old" ] && \ diff --git a/testcases/network/busy_poll/busy_poll02.sh b/testcases/network/busy_poll/busy_poll02.sh index ebae4d2f5..634bbd6bd 100755 --- a/testcases/network/busy_poll/busy_poll02.sh +++ b/testcases/network/busy_poll/busy_poll02.sh @@ -4,6 +4,9 @@ # # Author: Alexey Kodanev <alexey.kodanev@oracle.com> +TST_SETUP="setup" +TST_CLEANUP="cleanup" + cleanup() { [ -n "$busy_poll_old" ] && \ diff --git a/testcases/network/busy_poll/busy_poll03.sh b/testcases/network/busy_poll/busy_poll03.sh index 04d5978f7..b2e1c0a7a 100755 --- a/testcases/network/busy_poll/busy_poll03.sh +++ b/testcases/network/busy_poll/busy_poll03.sh @@ -4,6 +4,8 @@ # # Author: Alexey Kodanev <alexey.kodanev@oracle.com> +TST_SETUP="setup" +TST_CLEANUP="cleanup" TST_TEST_DATA="udp udp_lite" cleanup() diff --git a/testcases/network/busy_poll/busy_poll_lib.sh b/testcases/network/busy_poll/busy_poll_lib.sh index de61d3fcd..446ae3d65 100755 --- a/testcases/network/busy_poll/busy_poll_lib.sh +++ b/testcases/network/busy_poll/busy_poll_lib.sh @@ -1,10 +1,9 @@ #!/bin/sh # SPDX-License-Identifier: GPL-2.0-or-later +# Copyright (c) Linux Test Project, 2016-2022 # Copyright (c) 2016-2018 Oracle and/or its affiliates. All Rights Reserved. -TST_SETUP="setup" TST_TESTFUNC="test" -TST_CLEANUP="cleanup" TST_MIN_KVER="3.11" TST_NEEDS_TMPDIR=1 TST_NEEDS_ROOT=1
Although having variables in both busy_poll_lib.sh and the tests which are using it isn't optimal, hooking up callbacks on the reverse end of include is even worse code. Suggested-by: Martin Doucha <mdoucha@suse.cz> Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Follow-up of "v3 shell: Cleanup getopts usage" patchset [1], replacing first commit. Kind regards, Petr [1] https://patchwork.ozlabs.org/project/ltp/list/?series=297175 testcases/network/busy_poll/busy_poll01.sh | 3 +++ testcases/network/busy_poll/busy_poll02.sh | 3 +++ testcases/network/busy_poll/busy_poll03.sh | 2 ++ testcases/network/busy_poll/busy_poll_lib.sh | 3 +-- 4 files changed, 9 insertions(+), 2 deletions(-)