[1/3] lib/tst_net.sh: Append 6 to the end of $TST_OPTS

Message ID 1525663451-32016-1-git-send-email-yangx.jy@cn.fujitsu.com
State Rejected
Delegated to: Petr Vorel
Headers show
Series
  • [1/3] lib/tst_net.sh: Append 6 to the end of $TST_OPTS
Related show

Commit Message

Xiao Yang May 7, 2018, 3:24 a.m.
If the first character of optstring is set to a colon(tcp_fastopen_run.sh,
nfs_lib.sh), getopts should be in silent mode rather than process it as
an argument of 6.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/lib/tst_net.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Petr Vorel May 7, 2018, 6:43 a.m. | #1
Hi,

> If the first character of optstring is set to a colon(tcp_fastopen_run.sh,
> nfs_lib.sh), getopts should be in silent mode rather than process it as
> an argument of 6.

> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  testcases/lib/tst_net.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> index 3a0fe01..32b4f09 100644
> --- a/testcases/lib/tst_net.sh
> +++ b/testcases/lib/tst_net.sh
> @@ -19,7 +19,7 @@
>  # Author: Alexey Kodanev <alexey.kodanev@oracle.com>


> -TST_OPTS="6$TST_OPTS"
> +TST_OPTS="${TST_OPTS}6"
>  TST_PARSE_ARGS_CALLER="$TST_PARSE_ARGS"
>  TST_PARSE_ARGS="tst_net_parse_args"
>  TST_USAGE_CALLER="$TST_USAGE"

Acked-by: Petr Vorel <pvorel@suse.cz>

Good catch. Although I propose to get rid of ':' at the beginning for users of new shell
API (as it's IMHO better to see errors).


Kind regards,
Petr
Xiao Yang May 7, 2018, 7:30 a.m. | #2
On 2018/05/07 14:43, Petr Vorel wrote:
> Hi,
>
>> If the first character of optstring is set to a colon(tcp_fastopen_run.sh,
>> nfs_lib.sh), getopts should be in silent mode rather than process it as
>> an argument of 6.
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   testcases/lib/tst_net.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
>> index 3a0fe01..32b4f09 100644
>> --- a/testcases/lib/tst_net.sh
>> +++ b/testcases/lib/tst_net.sh
>> @@ -19,7 +19,7 @@
>>   # Author: Alexey Kodanev<alexey.kodanev@oracle.com>
>
>> -TST_OPTS="6$TST_OPTS"
>> +TST_OPTS="${TST_OPTS}6"
>>   TST_PARSE_ARGS_CALLER="$TST_PARSE_ARGS"
>>   TST_PARSE_ARGS="tst_net_parse_args"
>>   TST_USAGE_CALLER="$TST_USAGE"
> Acked-by: Petr Vorel<pvorel@suse.cz>
>
> Good catch. Although I propose to get rid of ':' at the beginning for users of new shell
> API (as it's IMHO better to see errors).
Hi Petr,

If getopts is in silent mode,  the invalid option is  placed  in OPTARG, 
so we can see errors by printing
the value of OPTARG.  But it is reasonable for me to get rid of ':' at 
the beginning of optstring. :-)

Thanks,
Xiao Yang
>
> Kind regards,
> Petr
>
>
> .
>
Petr Vorel May 7, 2018, 8:09 a.m. | #3
Hi Xiao,

> > Good catch. Although I propose to get rid of ':' at the beginning for users of new shell
> > API (as it's IMHO better to see errors).
> Hi Petr,

> If getopts is in silent mode,  the invalid option is  placed  in OPTARG, so
> we can see errors by printing
> the value of OPTARG.  But it is reasonable for me to get rid of ':' at the
> beginning of optstring. :-)
I know, TBROK inform us, so it can stay how it is for legacy API.
I meant it for the new API, where is going to be removed (the TBROK as well) as error is
handled in tst_run in tst_test.sh.

> Thanks,
> Xiao Yang


Kind regards,
Petr Vorel
Alexey Kodanev May 7, 2018, 10:31 a.m. | #4
On 05/07/2018 11:09 AM, Petr Vorel wrote:
> Hi Xiao,
> 
>>> Good catch. Although I propose to get rid of ':' at the beginning for users of new shell
>>> API (as it's IMHO better to see errors).
>> Hi Petr,
> 
>> If getopts is in silent mode,  the invalid option is  placed  in OPTARG, so
>> we can see errors by printing
>> the value of OPTARG.  But it is reasonable for me to get rid of ':' at the
>> beginning of optstring. :-)
> I know, TBROK inform us, so it can stay how it is for legacy API.
> I meant it for the new API, where is going to be removed (the TBROK as well) as error is
> handled in tst_run in tst_test.sh.
> 


What about silence error in tst_test.sh? And remove preceding column in the
tests (tcp_fastopen.sh, nfs_lib.sh). We handle the error already there, in
tst_run (and in old API the scripts have TBROK), so there is no point
printing one more redundant message that doesn't have LTP format.

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 8d49d34..edac619 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -265,11 +265,12 @@ tst_run()

        OPTIND=1

-       while getopts "hi:$TST_OPTS" name $TST_ARGS; do
+       while getopts ":hi:$TST_OPTS" name $TST_ARGS; do
                case $name in
                'h') tst_usage; exit 0;;
                'i') TST_ITERATIONS=$OPTARG;;
-               '?') tst_usage; exit 2;;
+               '?') tst_usage
+                    tst_brk TBROK "invalid option: '$OPTARG'";;
                *) $TST_PARSE_ARGS "$name" "$OPTARG";;
                esac
Petr Vorel May 7, 2018, 10:53 a.m. | #5
Hi,

> On 05/07/2018 11:09 AM, Petr Vorel wrote:
> > Hi Xiao,

> >>> Good catch. Although I propose to get rid of ':' at the beginning for users of new shell
> >>> API (as it's IMHO better to see errors).
> >> Hi Petr,

> >> If getopts is in silent mode,  the invalid option is  placed  in OPTARG, so
> >> we can see errors by printing
> >> the value of OPTARG.  But it is reasonable for me to get rid of ':' at the
> >> beginning of optstring. :-)
> > I know, TBROK inform us, so it can stay how it is for legacy API.
> > I meant it for the new API, where is going to be removed (the TBROK as well) as error is
> > handled in tst_run in tst_test.sh.



> What about silence error in tst_test.sh? And remove preceding column in the
> tests (tcp_fastopen.sh, nfs_lib.sh). We handle the error already there, in
> tst_run (and in old API the scripts have TBROK), so there is no point
> printing one more redundant message that doesn't have LTP format.

> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 8d49d34..edac619 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -265,11 +265,12 @@ tst_run()

>         OPTIND=1

> -       while getopts "hi:$TST_OPTS" name $TST_ARGS; do
> +       while getopts ":hi:$TST_OPTS" name $TST_ARGS; do
>                 case $name in
>                 'h') tst_usage; exit 0;;
>                 'i') TST_ITERATIONS=$OPTARG;;
> -               '?') tst_usage; exit 2;;
> +               '?') tst_usage
> +                    tst_brk TBROK "invalid option: '$OPTARG'";;
>                 *) $TST_PARSE_ARGS "$name" "$OPTARG";;
>                 esac
+1 as it makes sense to unify things.
It's just a bit faster to have printed the name of the script (if we don't use ':').

BTW this change affect only tests using new API (i.e. not tcp_fastopen_run.sh, nor nfs_lib.sh which still use legacy API).
I'm working on rewriting tests into new API.


Kind regards,
Petr

Patch

diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
index 3a0fe01..32b4f09 100644
--- a/testcases/lib/tst_net.sh
+++ b/testcases/lib/tst_net.sh
@@ -19,7 +19,7 @@ 
 # Author: Alexey Kodanev <alexey.kodanev@oracle.com>
 #
 
-TST_OPTS="6$TST_OPTS"
+TST_OPTS="${TST_OPTS}6"
 TST_PARSE_ARGS_CALLER="$TST_PARSE_ARGS"
 TST_PARSE_ARGS="tst_net_parse_args"
 TST_USAGE_CALLER="$TST_USAGE"