* [PATCH bpf-next 0/2] bpf/selftests: Small vmtest.sh fixes
@ 2022-08-09 17:11 Daniel Xu
2022-08-09 17:11 ` [PATCH bpf-next 1/2] selftests/bpf: Fix vmtest.sh -h to not require root Daniel Xu
2022-08-09 17:11 ` [PATCH bpf-next 2/2] selftests/bpf: Fix vmtest.sh getopts optstring Daniel Xu
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Xu @ 2022-08-09 17:11 UTC (permalink / raw)
To: bpf, ast, daniel, andrii, kpsingh; +Cc: Daniel Xu, linux-kernel
Two small quality of life fixes for vmtest.sh:
* Don't require root for `-h`
* Fix unset variable errors for failed option parsing
Daniel Xu (2):
selftests/bpf: Fix vmtest.sh -h to not require root
selftests/bpf: Fix vmtest.sh getopts optstring
tools/testing/selftests/bpf/vmtest.sh | 34 +++++++++++++--------------
1 file changed, 17 insertions(+), 17 deletions(-)
--
2.37.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/2] selftests/bpf: Fix vmtest.sh -h to not require root
2022-08-09 17:11 [PATCH bpf-next 0/2] bpf/selftests: Small vmtest.sh fixes Daniel Xu
@ 2022-08-09 17:11 ` Daniel Xu
2022-08-09 18:19 ` Daniel Müller
2022-08-09 17:11 ` [PATCH bpf-next 2/2] selftests/bpf: Fix vmtest.sh getopts optstring Daniel Xu
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Xu @ 2022-08-09 17:11 UTC (permalink / raw)
To: bpf, ast, daniel, andrii, kpsingh; +Cc: Daniel Xu, linux-kernel
Set the exit trap only after argument parsing is done. This way argument
parse failure or `-h` will not require sudo.
Reasoning is that it's confusing that a help message would require root
access.
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
tools/testing/selftests/bpf/vmtest.sh | 32 +++++++++++++--------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/bpf/vmtest.sh b/tools/testing/selftests/bpf/vmtest.sh
index b86ae4a2e5c5..976ef7585b33 100755
--- a/tools/testing/selftests/bpf/vmtest.sh
+++ b/tools/testing/selftests/bpf/vmtest.sh
@@ -307,6 +307,20 @@ update_kconfig()
fi
}
+catch()
+{
+ local exit_code=$1
+ local exit_status_file="${OUTPUT_DIR}/${EXIT_STATUS_FILE}"
+ # This is just a cleanup and the directory may
+ # have already been unmounted. So, don't let this
+ # clobber the error code we intend to return.
+ unmount_image || true
+ if [[ -f "${exit_status_file}" ]]; then
+ exit_code="$(cat ${exit_status_file})"
+ fi
+ exit ${exit_code}
+}
+
main()
{
local script_dir="$(cd -P -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)"
@@ -353,6 +367,8 @@ main()
done
shift $((OPTIND -1))
+ trap 'catch "$?"' EXIT
+
if [[ $# -eq 0 && "${debug_shell}" == "no" ]]; then
echo "No command specified, will run ${DEFAULT_COMMAND} in the vm"
else
@@ -409,20 +425,4 @@ main()
fi
}
-catch()
-{
- local exit_code=$1
- local exit_status_file="${OUTPUT_DIR}/${EXIT_STATUS_FILE}"
- # This is just a cleanup and the directory may
- # have already been unmounted. So, don't let this
- # clobber the error code we intend to return.
- unmount_image || true
- if [[ -f "${exit_status_file}" ]]; then
- exit_code="$(cat ${exit_status_file})"
- fi
- exit ${exit_code}
-}
-
-trap 'catch "$?"' EXIT
-
main "$@"
--
2.37.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 1/2] selftests/bpf: Fix vmtest.sh -h to not require root
2022-08-09 17:11 ` [PATCH bpf-next 1/2] selftests/bpf: Fix vmtest.sh -h to not require root Daniel Xu
@ 2022-08-09 18:19 ` Daniel Müller
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Müller @ 2022-08-09 18:19 UTC (permalink / raw)
To: Daniel Xu; +Cc: bpf, ast, daniel, andrii, kpsingh, linux-kernel
On Tue, Aug 09, 2022 at 11:11:09AM -0600, Daniel Xu wrote:
> Set the exit trap only after argument parsing is done. This way argument
> parse failure or `-h` will not require sudo.
>
> Reasoning is that it's confusing that a help message would require root
> access.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
> tools/testing/selftests/bpf/vmtest.sh | 32 +++++++++++++--------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/vmtest.sh b/tools/testing/selftests/bpf/vmtest.sh
> index b86ae4a2e5c5..976ef7585b33 100755
> --- a/tools/testing/selftests/bpf/vmtest.sh
> +++ b/tools/testing/selftests/bpf/vmtest.sh
> @@ -307,6 +307,20 @@ update_kconfig()
> fi
> }
>
> +catch()
> +{
> + local exit_code=$1
> + local exit_status_file="${OUTPUT_DIR}/${EXIT_STATUS_FILE}"
> + # This is just a cleanup and the directory may
> + # have already been unmounted. So, don't let this
> + # clobber the error code we intend to return.
> + unmount_image || true
> + if [[ -f "${exit_status_file}" ]]; then
> + exit_code="$(cat ${exit_status_file})"
> + fi
> + exit ${exit_code}
> +}
> +
> main()
> {
> local script_dir="$(cd -P -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)"
> @@ -353,6 +367,8 @@ main()
> done
> shift $((OPTIND -1))
>
> + trap 'catch "$?"' EXIT
> +
> if [[ $# -eq 0 && "${debug_shell}" == "no" ]]; then
> echo "No command specified, will run ${DEFAULT_COMMAND} in the vm"
> else
> @@ -409,20 +425,4 @@ main()
> fi
> }
>
> -catch()
> -{
> - local exit_code=$1
> - local exit_status_file="${OUTPUT_DIR}/${EXIT_STATUS_FILE}"
> - # This is just a cleanup and the directory may
> - # have already been unmounted. So, don't let this
> - # clobber the error code we intend to return.
> - unmount_image || true
> - if [[ -f "${exit_status_file}" ]]; then
> - exit_code="$(cat ${exit_status_file})"
> - fi
> - exit ${exit_code}
> -}
> -
> -trap 'catch "$?"' EXIT
> -
> main "$@"
> --
> 2.37.1
>
Makes sense and looks good to me.
Acked-by: Daniel Müller <deso@posteo.net>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Fix vmtest.sh getopts optstring
2022-08-09 17:11 [PATCH bpf-next 0/2] bpf/selftests: Small vmtest.sh fixes Daniel Xu
2022-08-09 17:11 ` [PATCH bpf-next 1/2] selftests/bpf: Fix vmtest.sh -h to not require root Daniel Xu
@ 2022-08-09 17:11 ` Daniel Xu
2022-08-09 18:18 ` Daniel Müller
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Xu @ 2022-08-09 17:11 UTC (permalink / raw)
To: bpf, ast, daniel, andrii, kpsingh; +Cc: Daniel Xu, linux-kernel
Before, you could see the following errors:
```
$ ./vmtest.sh -j
./vmtest.sh: option requires an argument -- j
./vmtest.sh: line 357: OPTARG: unbound variable
$ ./vmtest.sh -z
./vmtest.sh: illegal option -- z
./vmtest.sh: line 357: OPTARG: unbound variable
```
Fix by adding ':' as first character of optstring. Reason is that
getopts requires ':' as the first character for OPTARG to be set in the
`?` and `:` error cases.
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
tools/testing/selftests/bpf/vmtest.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/vmtest.sh b/tools/testing/selftests/bpf/vmtest.sh
index 976ef7585b33..a29aa05ebb3e 100755
--- a/tools/testing/selftests/bpf/vmtest.sh
+++ b/tools/testing/selftests/bpf/vmtest.sh
@@ -333,7 +333,7 @@ main()
local exit_command="poweroff -f"
local debug_shell="no"
- while getopts 'hskid:j:' opt; do
+ while getopts ':hskid:j:' opt; do
case ${opt} in
i)
update_image="yes"
--
2.37.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 2/2] selftests/bpf: Fix vmtest.sh getopts optstring
2022-08-09 17:11 ` [PATCH bpf-next 2/2] selftests/bpf: Fix vmtest.sh getopts optstring Daniel Xu
@ 2022-08-09 18:18 ` Daniel Müller
2022-08-09 20:34 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Müller @ 2022-08-09 18:18 UTC (permalink / raw)
To: Daniel Xu; +Cc: bpf, ast, daniel, andrii, kpsingh, linux-kernel
On Tue, Aug 09, 2022 at 11:11:10AM -0600, Daniel Xu wrote:
> Before, you could see the following errors:
>
> ```
> $ ./vmtest.sh -j
> ./vmtest.sh: option requires an argument -- j
> ./vmtest.sh: line 357: OPTARG: unbound variable
>
> $ ./vmtest.sh -z
> ./vmtest.sh: illegal option -- z
> ./vmtest.sh: line 357: OPTARG: unbound variable
> ```
>
> Fix by adding ':' as first character of optstring. Reason is that
> getopts requires ':' as the first character for OPTARG to be set in the
> `?` and `:` error cases.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
> tools/testing/selftests/bpf/vmtest.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/vmtest.sh b/tools/testing/selftests/bpf/vmtest.sh
> index 976ef7585b33..a29aa05ebb3e 100755
> --- a/tools/testing/selftests/bpf/vmtest.sh
> +++ b/tools/testing/selftests/bpf/vmtest.sh
> @@ -333,7 +333,7 @@ main()
> local exit_command="poweroff -f"
> local debug_shell="no"
>
> - while getopts 'hskid:j:' opt; do
> + while getopts ':hskid:j:' opt; do
> case ${opt} in
> i)
> update_image="yes"
> --
> 2.37.1
>
I tested with this change and it worked fine for me. One thing to consider
pointing out more clearly in the description is that ':' as the first character
of the optstring switches getopts to silent mode. The desire to run in this mode
seems to have been there all along, as the script takes care of reporting
errors.
Acked-by: Daniel Müller <deso@posteo.net>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 2/2] selftests/bpf: Fix vmtest.sh getopts optstring
2022-08-09 18:18 ` Daniel Müller
@ 2022-08-09 20:34 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2022-08-09 20:34 UTC (permalink / raw)
To: Daniel Müller, Daniel Xu; +Cc: bpf, ast, andrii, kpsingh, linux-kernel
On 8/9/22 8:18 PM, Daniel Müller wrote:
> On Tue, Aug 09, 2022 at 11:11:10AM -0600, Daniel Xu wrote:
>> Before, you could see the following errors:
>>
>> ```
>> $ ./vmtest.sh -j
>> ./vmtest.sh: option requires an argument -- j
>> ./vmtest.sh: line 357: OPTARG: unbound variable
>>
>> $ ./vmtest.sh -z
>> ./vmtest.sh: illegal option -- z
>> ./vmtest.sh: line 357: OPTARG: unbound variable
>> ```
>>
>> Fix by adding ':' as first character of optstring. Reason is that
>> getopts requires ':' as the first character for OPTARG to be set in the
>> `?` and `:` error cases.
>>
>> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>> ---
>> tools/testing/selftests/bpf/vmtest.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/vmtest.sh b/tools/testing/selftests/bpf/vmtest.sh
>> index 976ef7585b33..a29aa05ebb3e 100755
>> --- a/tools/testing/selftests/bpf/vmtest.sh
>> +++ b/tools/testing/selftests/bpf/vmtest.sh
>> @@ -333,7 +333,7 @@ main()
>> local exit_command="poweroff -f"
>> local debug_shell="no"
>>
>> - while getopts 'hskid:j:' opt; do
>> + while getopts ':hskid:j:' opt; do
>> case ${opt} in
>> i)
>> update_image="yes"
>
> I tested with this change and it worked fine for me. One thing to consider
> pointing out more clearly in the description is that ':' as the first character
> of the optstring switches getopts to silent mode. The desire to run in this mode
> seems to have been there all along, as the script takes care of reporting
> errors.
I've added this description to the commit message as well for future reference
while applying. Thanks everyone!
> Acked-by: Daniel Müller <deso@posteo.net>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-09 20:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-09 17:11 [PATCH bpf-next 0/2] bpf/selftests: Small vmtest.sh fixes Daniel Xu
2022-08-09 17:11 ` [PATCH bpf-next 1/2] selftests/bpf: Fix vmtest.sh -h to not require root Daniel Xu
2022-08-09 18:19 ` Daniel Müller
2022-08-09 17:11 ` [PATCH bpf-next 2/2] selftests/bpf: Fix vmtest.sh getopts optstring Daniel Xu
2022-08-09 18:18 ` Daniel Müller
2022-08-09 20:34 ` Daniel Borkmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox