| Message ID | 20211214144309.6704-3-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add support for debugging .all_filesystems | expand |
> -----Original Message----- > From: ltp <ltp-bounces+tim.bird=sony.com@lists.linux.it> On Behalf Of Petr Vorel > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > new in v2 > > lib/tst_test.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/lib/tst_test.c b/lib/tst_test.c > index 265df6543b..f92ff858e9 100644 > --- a/lib/tst_test.c > +++ b/lib/tst_test.c > @@ -475,6 +475,19 @@ static void print_help(void) > { > unsigned int i; > > + /* see doc/user-guide.txt, which lists also shell API variables */ > + fprintf(stderr, "Environment Variables\n"); > + fprintf(stderr, "---------------------\n"); > + fprintf(stderr, "KCONFIG_PATH Specify kernel config file\n"); > + fprintf(stderr, "LTPROOT Prefix for installed LTP, the default is /opt/ltp\n"); > + fprintf(stderr, "LTP_COLORIZE_OUTPUT Force colorized output behaviour (y/1 always, n/0: never)\n"); > + fprintf(stderr, "LTP_DEV Path to the block device to be used (for .needs_device)\n"); > + fprintf(stderr, "LTP_DEV_FS_TYPE Filesystem used for testing (default: %s)\n", DEFAULT_FS_TYPE); > + fprintf(stderr, "LTP_TIMEOUT_MUL Multiply timeout (must be number >= 1)\n"); I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer) > + fprintf(stderr, "LTP_VIRT_OVERRIDE Overrides virtual machine detection (values: \"\"|kvm|microsoft|xen|zvm\n"); > + fprintf(stderr, "TMPDIR Base directory for template directory (for .needs_tmpdir, default: %s)\n", TEMPDIR); > + fprintf(stderr, "\n"); > + > fprintf(stderr, "Options\n"); > fprintf(stderr, "-------\n"); > > -- > 2.34.1 -- Tim
Hi Tim, > > + fprintf(stderr, "LTP_TIMEOUT_MUL Multiply timeout (must be number >= 1)\n"); > I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer) Also makes sense, thanks! Kind regards, Petr
> Hi Tim, > > > + fprintf(stderr, "LTP_TIMEOUT_MUL Multiply timeout (must be number >= 1)\n"); > > I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer) > Also makes sense, thanks! Although it does not have to be integer. => "Timeout multiplier (must be a number >=1" For C API it's used any value: ./open01 tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s LTP_TIMEOUT_MUL=1.15 ./open01 tst_test.c:1409: TINFO: Timeout per run is 0h 05m 45s For shell API: ./zram02.sh zram02 1 TINFO: timeout per run is 0h 5m 0s zram02 1 TINFO: timeout per run is 0h 7m 30s LTP_TIMEOUT_MUL=1.15 ./zram02.sh zram02 1 TINFO: ceiling LTP_TIMEOUT_MUL to 2 zram02 1 TINFO: timeout per run is 0h 10m 0s zram02 1 TINFO: timeout per run is 0h 15m 0s Doc [1] explain it: Variable is also used in shell tests, but ceiled to int. Kind regards, Petr [1] https://github.com/linux-test-project/ltp/wiki/User-Guidelines > Kind regards, > Petr
> -----Original Message----- > From: Petr Vorel <pvorel@suse.cz> > > > Hi Tim, > > > > > + fprintf(stderr, "LTP_TIMEOUT_MUL Multiply timeout (must be number >= 1)\n"); > > > > I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer) > > > Also makes sense, thanks! > Although it does not have to be integer. > => "Timeout multiplier (must be a number >=1" > > For C API it's used any value: > ./open01 > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s > > LTP_TIMEOUT_MUL=1.15 ./open01 > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 45s > When I grepped the source code I saw the usage in testcases/lib/tst_test.sh, but somehow missed the usage in lib/tst_test.c (sloppy on my part!) Thanks for pointing this out. > For shell API: > ./zram02.sh > zram02 1 TINFO: timeout per run is 0h 5m 0s > zram02 1 TINFO: timeout per run is 0h 7m 30s > > LTP_TIMEOUT_MUL=1.15 ./zram02.sh > zram02 1 TINFO: ceiling LTP_TIMEOUT_MUL to 2 > zram02 1 TINFO: timeout per run is 0h 10m 0s > zram02 1 TINFO: timeout per run is 0h 15m 0s > > Doc [1] explain it: Variable is also used in shell tests, but ceiled to int. > Would it be useful to add a bit of code to tst_test.sh to handle floating point? The shell construct of '$((timeout * LTP_TIMEOUT_MUL))' can't handle floating point, but floating point could be done pretty easily with a callout to awk or bc. I'm willing to look at this and submit a patch. But does the shell test system try to avoid dependencies on other utility programs (like awk or bc)? Maybe rounding timeouts up is preferred behavior anyway, so this is not needed. What do you think? In any event, I retract my "should be an integer" suggestion. :-) -- Tim
Hi Tim, > > -----Original Message----- > > From: Petr Vorel <pvorel@suse.cz> > > > Hi Tim, > > > > > + fprintf(stderr, "LTP_TIMEOUT_MUL Multiply timeout (must be number >= 1)\n"); > > > > I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer) > > > Also makes sense, thanks! > > Although it does not have to be integer. > > => "Timeout multiplier (must be a number >=1" > > For C API it's used any value: > > ./open01 > > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s > > LTP_TIMEOUT_MUL=1.15 ./open01 > > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 45s > When I grepped the source code I saw the usage in testcases/lib/tst_test.sh, > but somehow missed the usage in lib/tst_test.c (sloppy on my part!) FYI this help is for C API. We should print variables also in shell API, but I wanted to keep this patchset small. > Thanks for pointing this out. > > For shell API: > > ./zram02.sh > > zram02 1 TINFO: timeout per run is 0h 5m 0s > > zram02 1 TINFO: timeout per run is 0h 7m 30s > > LTP_TIMEOUT_MUL=1.15 ./zram02.sh > > zram02 1 TINFO: ceiling LTP_TIMEOUT_MUL to 2 > > zram02 1 TINFO: timeout per run is 0h 10m 0s > > zram02 1 TINFO: timeout per run is 0h 15m 0s > > Doc [1] explain it: Variable is also used in shell tests, but ceiled to int. > Would it be useful to add a bit of code to tst_test.sh to handle > floating point? The shell construct of '$((timeout * LTP_TIMEOUT_MUL))' > can't handle floating point, but floating point could be done pretty easily > with a callout to awk or bc. > I'm willing to look at this and submit a patch. But does the shell > test system try to avoid dependencies on other utility programs (like awk or bc)? > Maybe rounding timeouts up is preferred behavior anyway, so > this is not needed. What do you think? [ Cc Joerg, Li and Cyril, who were involved in shell timeout ] We spent quite a lot of time before we end-up with ceiling, I'd have to search for the reasons. We also didn't think that it's a big problem to not being precise on shell. But feel free to improve things. Just, generally we prefer to wrote small C code instead adding new dependencies (bc etc) or trying to make shell portable. Actually writing C parser would be few lines of code. > In any event, I retract my "should be an integer" suggestion. :-) You pointed out also wrong spelling, which I'm going to update. Thanks a lot for your review! Kind regards, Petr > -- Tim
Hi Tim, Petr, On Wed, Dec 15, 2021 at 2:54 AM Petr Vorel <pvorel@suse.cz> wrote: > > > Would it be useful to add a bit of code to tst_test.sh to handle > > floating point? The shell construct of '$((timeout * LTP_TIMEOUT_MUL))' > > can't handle floating point, but floating point could be done pretty > easily > > with a callout to awk or bc. > > > I'm willing to look at this and submit a patch. But does the shell > > test system try to avoid dependencies on other utility programs (like > awk or bc)? > > > Maybe rounding timeouts up is preferred behavior anyway, so > > this is not needed. What do you think? > > [ Cc Joerg, Li and Cyril, who were involved in shell timeout ] > We spent quite a lot of time before we end-up with ceiling, I'd have to > search > for the reasons. We also didn't think that it's a big problem to not being > precise on shell. But feel free to improve things. Just, generally we > prefer to > Yeah, I agree with this. To be honest, I personally have no thoughts to make the shell to handle float point, and by now I don't see any person request that, for most situations we just use an round up number to satisfy ourselves. But anyway, I have no objection to improving that. Feel free to submit a patch :). > wrote small C code instead adding new dependencies (bc etc) or trying to > make > shell portable. Actually writing C parser would be few lines of code. > +1 For patch itself: Reviewed-by: Li Wang <liwang@redhat.com>
Hi! > [ Cc Joerg, Li and Cyril, who were involved in shell timeout ] > We spent quite a lot of time before we end-up with ceiling, I'd have to search > for the reasons. We also didn't think that it's a big problem to not being > precise on shell. But feel free to improve things. Just, generally we prefer to > wrote small C code instead adding new dependencies (bc etc) or trying to make > shell portable. Actually writing C parser would be few lines of code. We even have tst_multiply_timeout() function in the C API that does all the work. So this should be as simple as adding another C helper that would take one number on commandline as a parameter and print the result after calling the function.
diff --git a/lib/tst_test.c b/lib/tst_test.c index 265df6543b..f92ff858e9 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -475,6 +475,19 @@ static void print_help(void) { unsigned int i; + /* see doc/user-guide.txt, which lists also shell API variables */ + fprintf(stderr, "Environment Variables\n"); + fprintf(stderr, "---------------------\n"); + fprintf(stderr, "KCONFIG_PATH Specify kernel config file\n"); + fprintf(stderr, "LTPROOT Prefix for installed LTP, the default is /opt/ltp\n"); + fprintf(stderr, "LTP_COLORIZE_OUTPUT Force colorized output behaviour (y/1 always, n/0: never)\n"); + fprintf(stderr, "LTP_DEV Path to the block device to be used (for .needs_device)\n"); + fprintf(stderr, "LTP_DEV_FS_TYPE Filesystem used for testing (default: %s)\n", DEFAULT_FS_TYPE); + fprintf(stderr, "LTP_TIMEOUT_MUL Multiply timeout (must be number >= 1)\n"); + fprintf(stderr, "LTP_VIRT_OVERRIDE Overrides virtual machine detection (values: \"\"|kvm|microsoft|xen|zvm\n"); + fprintf(stderr, "TMPDIR Base directory for template directory (for .needs_tmpdir, default: %s)\n", TEMPDIR); + fprintf(stderr, "\n"); + fprintf(stderr, "Options\n"); fprintf(stderr, "-------\n");
Signed-off-by: Petr Vorel <pvorel@suse.cz> --- new in v2 lib/tst_test.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)