[v6,3/3] check-module-params: Introduce check-module-params.sh
Commit Message
One part of maintaining backwards compatibility with older
userspace programs is avoiding changes to module parameters.
To that end, add a script (check-module-params.sh) which
performs a simple check of module parameter changes across
git references.
For example, if this module parameter:
module_param(max_nullfunc_tries, int, 0644);
...restricted its mode parameter:
module_param(max_nullfunc_tries, int, 0600);
The script would flag the change:
Module parameter "max_nullfunc_tries" in net/mac80211/mlme.c changed!
Original args: int,0644
New args: int,0600
Signed-off-by: John Moon <quic_johmoo@quicinc.com>
---
scripts/check-module-params.sh | 295 +++++++++++++++++++++++++++++++++
1 file changed, 295 insertions(+)
create mode 100755 scripts/check-module-params.sh
--
2.17.1
Comments
On Sat, Oct 28, 2023 at 4:31 AM John Moon <quic_johmoo@quicinc.com> wrote:
>
> One part of maintaining backwards compatibility with older
> userspace programs is avoiding changes to module parameters.
>
> To that end, add a script (check-module-params.sh) which
> performs a simple check of module parameter changes across
> git references.
>
> For example, if this module parameter:
>
> module_param(max_nullfunc_tries, int, 0644);
>
> ...restricted its mode parameter:
>
> module_param(max_nullfunc_tries, int, 0600);
>
> The script would flag the change:
>
> Module parameter "max_nullfunc_tries" in net/mac80211/mlme.c changed!
> Original args: int,0644
> New args: int,0600
I know this is just a simple diff, and we cannot expect
accuracy from this tool.
I just tried
$ ./scripts/check-module-params.sh -b v6.7-rc1 -p v6.6
Then, I got
error - 21/576 files with modules parameters appear _not_ to be
backwards compatible
In my understanding, it includes many false alarms.
In most of the cases, the driver was just removed.
[pattern 1] Driver was removed
Module parameter "sal_rec_max" in arch/ia64/kernel/mca_drv.c removed!
or
Module parameter "up_delay" in
drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c removed!
[pattern 2] cosmetic change
Module parameter "nested" in arch/x86/kvm/vmx/vmx.c changed!
Original args: bool,S_IRUGO
New args: bool,0444
But, it sometimes catches real ones:
Module parameter "ublks_max" in drivers/block/ublk_drv.c changed!
Original args: int,0444
New args: &ublk_max_ublks_ops,&ublks_max,0644
--> Need a close check
Module parameter "vm_debug" in drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c removed!
Original args: amdgpu_vm_debug,int,0644
--> 887db1e49a73 is a breakage, but it seems to be intentional
according to the commit log
> Signed-off-by: John Moon <quic_johmoo@quicinc.com>
> ---
> scripts/check-module-params.sh | 295 +++++++++++++++++++++++++++++++++
> 1 file changed, 295 insertions(+)
> create mode 100755 scripts/check-module-params.sh
>
> diff --git a/scripts/check-module-params.sh b/scripts/check-module-params.sh
> new file mode 100755
> index 000000000000..4d2b2cd483e8
> --- /dev/null
> +++ b/scripts/check-module-params.sh
> @@ -0,0 +1,295 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-only
> +# Script to check commits for UAPI backwards compatibility
> +
> +set -o errexit
> +set -o pipefail
> +
> +print_usage() {
> + name=$(basename "$0")
> + cat << EOF
> +$name - check for module parameters stability across git commits.
> +
> +By default, the script will check to make sure the latest commit (or current
> +dirty changes) did not introduce changes when compared to HEAD^1. You can
> +check against additional commit ranges with the -b and -p options.
> +
> +Usage: $name [-b BASE_REF] [-p PAST_REF] [-j N] [-l ERROR_LOG] [-q] [-v]
> +
> +Options:
> + -b BASE_REF Base git reference to use for comparison. If unspecified or empty,
> + will use any dirty changes in tree to UAPI files. If there are no
> + dirty changes, HEAD will be used.
> + -p PAST_REF Compare BASE_REF to PAST_REF (e.g. -p v6.1). If unspecified or empty,
> + will use BASE_REF^1. Must be an ancestor of BASE_REF. Only headers
> + that exist on PAST_REF will be checked for compatibility.
> + -j JOBS Number of checks to run in parallel (default: number of CPU cores).
> + -l ERROR_LOG Write error log to file (default: no error log is generated).
> + -q Quiet operation (suppress stdout, still print stderr).
> + -v Verbose operation (print more information about each header being checked).
> +
> +Exit codes:
> + $SUCCESS) Success
> + $FAILURE) Module param differences detected
> +EOF
> +}
> +
> +readonly SUCCESS=0
> +readonly FAILURE=1
> +
> +# Print to stderr
> +eprintf() {
> + # shellcheck disable=SC2059
> + printf "$@" >&2
> +}
> +
> +# Check if git tree is dirty
> +tree_is_dirty() {
> + ! git diff --quiet
> +}
> +
> +file_module_params_unmodified() {
> + local file="$1"
> + local base_ref="$2"
> + local past_ref="$3"
> + local base_params_file="${TMP_DIR}/${file}.base"
> + local past_params_file="${TMP_DIR}/${file}.past"
> + local error_log="${TMP_DIR}/${file}.error"
> +
> + local -r awk_cmd='/^ *module_param.*\(/,/.*\);/'
> +
> + mkdir -p "$(dirname "$error_log")"
> + git show "${past_ref}:${file}" 2> /dev/null \
> + | awk "$awk_cmd" > "$past_params_file" || true
> +
> + # Ignore files that don't exist at the past ref or don't have module params
> + if [ ! -s "$past_params_file" ]; then
> + return 255 # Special return code for "no-op"
> + fi
> +
> + if [ -z "$base_ref" ]; then
> + awk "$awk_cmd" "${KERNEL_SRC}/${file}" \
> + > "$base_params_file" 2> /dev/null || true
> + else
> + git show "${base_ref}:${file}" 2> /dev/null \
> + | awk "$awk_cmd" > "$base_params_file" || true
> + fi
> +
> + # Process the param data to come up with an associative array of param names to param data
> + # For example:
> + # module_param_call(foo, set_result, get_result, NULL, 0600);
> + #
> + # is processed into:
> + # pre_change_params[foo]="set_result,get_result,NULL,0600"
> + local -A pre_change_params
> + local param_name
> + local param_params
> + while read -r mod_param_args; do
> + param_name="$(echo "$mod_param_args" | cut -d ',' -f 1)"
> + param_params="$(echo "$mod_param_args" | cut -d ',' -f 2-)"
> +
> + pre_change_params[$param_name]=$param_params
> + done < <(tr -d '\t\n ' < "$past_params_file" | tr ';' '\n' | grep -o '(.*)' | tr -d '()')
Maybe
grep -o '(.*)' | tr -d '()'
can become a single process,
sed 's/.*(\(.*\))/\1/'
> +
> + local -A post_change_params
> + while read -r mod_param_args; do
> + param_name="$(echo "$mod_param_args" | cut -d ',' -f 1)"
> + param_params="$(echo "$mod_param_args" | cut -d ',' -f 2-)"
> +
> + post_change_params[$param_name]=$param_params
> + done < <(tr -d '\t\n ' < "$base_params_file" | tr ';' '\n' | grep -o '(.*)' | tr -d '()')
> +
> + # Flag any module param changes that:
> + # - Remove/rename a parameter
> + # - Change the arguments of the parameter
> + local incompat_param_changes=0
> + local pre
> + local post
> + for param_name in "${!pre_change_params[@]}"; do
> + pre="${pre_change_params[$param_name]}"
> + if [ ! "${post_change_params[$param_name]+set}" ]; then
> + {
> + printf "Module parameter \"%s\" in %s removed!\n" "$param_name" "$file"
> + printf " Original args: %s\n" "$pre"
> + } > "$error_log"
> + incompat_param_changes=$((incompat_param_changes + 1))
> + continue
> + fi
> +
> + post="${post_change_params[$param_name]}"
> + if [ "$pre" != "$post" ]; then
> + {
> + printf "Module parameter \"%s\" in %s changed!\n" "$param_name" "$file"
> + printf " Original args: %s\n" "$pre"
> + printf " New args: %s\n" "$post"
> + } > "$error_log"
> + incompat_param_changes=$((incompat_param_changes + 1))
> + continue
> + fi
> + done
> +
> + if [ "$incompat_param_changes" -gt 0 ]; then
> + return 1
> + fi
> +}
> +
> +run() {
> + local base_ref="$1"
> + local past_ref="$2"
> + local abi_error_log="$3"
> +
> + diff_args=("$past_ref")
> + if [ -n "$base_ref" ]; then
> + diff_args+=("$base_ref")
> + fi
> +
> + local -a threads=()
> + local passed=0
> + local failed=0
> + printf "Checking files between %s and %s for module parameter compatibility...\n" \
> + "$past_ref" "$base_ref"
> + while read -r modified_file; do
> + if [ "${#threads[@]}" -ge "$MAX_THREADS" ]; then
> + wait "${threads[0]}" && ret="$?" || ret="$?"
> + if [ "$ret" -eq 0 ]; then
> + passed=$((passed + 1))
> + elif [ "$ret" -eq 1 ]; then
> + failed=$((failed + 1))
> + fi
> + threads=("${threads[@]:1}")
> + fi
> +
> + file_module_params_unmodified "$modified_file" "$base_ref" "$past_ref" &
> + threads+=("$!")
> + done < <(git diff --diff-filter=MCD --name-only "${diff_args[@]}" -- '*.c' '*.h')
> +
> + for t in "${threads[@]}"; do
> + wait "$t" && ret="$?" || ret="$?"
> + if [ "$ret" -eq 0 ]; then
> + passed=$((passed + 1))
> + elif [ "$ret" -eq 1 ]; then
> + failed=$((failed + 1))
> + fi
> + done
> +
> + total=$((passed + failed))
> + if [ "$total" -eq 0 ]; then
> + printf "No files with module parameters modified between %s and %s\n" \
> + "$past_ref" "${base_ref:-dirty tree}"
> + exit 0
> + fi
> +
> + if [ -n "$abi_error_log" ]; then
> + printf 'Generated by "%s %s" from git ref %s\n\n' \
> + "$0" "$*" "$(git rev-parse HEAD)" > "$abi_error_log"
> + fi
> +
> + while read -r error_file; do
> + {
> + cat "$error_file"
> + printf "\n\n"
> + } | tee -a "${abi_error_log:-/dev/null}" >&2
> + done < <(find "$TMP_DIR" -type f -name '*.error')
> +
> + if [ "$failed" -gt 0 ]; then
> + eprintf "error - %d/%d files with modules parameters appear _not_ to be backwards compatible\n" \
> + "$failed" "$total"
> + if [ -n "$abi_error_log" ]; then
> + eprintf "Failure summary saved to %s\n" "$abi_error_log"
> + fi
> + else
> + printf "All %d files with module_parameters checked appear to be backwards compatible\n" \
> + "$total" "$ARCH"
> + fi
> +
> + exit "$failed"
> +}
> +
> +# Make sure the git refs we have make sense
> +check_refs() {
> + if ! git rev-parse --is-inside-work-tree > /dev/null 2>&1; then
> + eprintf "error - this script requires the kernel tree to be initialized with Git\n"
> + return 1
> + fi
> +
> + if ! git rev-parse --verify "$past_ref" > /dev/null 2>&1; then
> + printf 'error - invalid git reference "%s"\n' "$past_ref"
> + return 1
> + fi
> +
> + if [ -n "$base_ref" ]; then
> + if ! git merge-base --is-ancestor "$past_ref" "$base_ref" > /dev/null 2>&1; then
> + printf 'error - "%s" is not an ancestor of base ref "%s"\n' "$past_ref" "$base_ref"
> + return 1
> + fi
> + if [ "$(git rev-parse "$base_ref")" = "$(git rev-parse "$past_ref")" ]; then
> + printf 'error - "%s" and "%s" are the same reference\n' "$past_ref" "$base_ref"
> + return 1
> + fi
> + fi
> +}
> +
> +main() {
> + MAX_THREADS=$(nproc)
> + quiet="false"
> + local base_ref=""
> + while getopts "hb:p:j:l:q" opt; do
> + case $opt in
> + h)
> + print_usage
> + exit "$SUCCESS"
> + ;;
> + b)
> + base_ref="$OPTARG"
> + ;;
> + p)
> + past_ref="$OPTARG"
> + ;;
> + j)
> + MAX_THREADS="$OPTARG"
> + ;;
> + l)
> + abi_error_log="$OPTARG"
> + ;;
> + q)
> + quiet="true"
> + ;;
> + *)
> + exit "$FAIL_PREREQ"
> + esac
> + done
> +
> + if [ "$quiet" = "true" ]; then
> + exec > /dev/null 2>&1
> + fi
> +
> + if [ -z "$KERNEL_SRC" ]; then
> + KERNEL_SRC="$(realpath "$(dirname "$0")"/..)"
> + fi
> +
> + cd "$KERNEL_SRC"
> +
> + if [ -z "$base_ref" ] && ! tree_is_dirty; then
> + base_ref=HEAD
> + fi
> +
> + if [ -z "$past_ref" ]; then
> + if [ -n "$base_ref" ]; then
> + past_ref="${base_ref}^1"
> + else
> + past_ref=HEAD
> + fi
> + fi
> +
> + if ! check_refs; then
> + exit "$FAIL_PREREQ"
> + fi
> +
> + TMP_DIR=$(mktemp -d)
> + readonly TMP_DIR
> + trap 'rm -rf "$TMP_DIR"' EXIT
> +
> + run "$base_ref" "$past_ref" "$abi_error_log"
> +}
> +
> +main "$@"
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
new file mode 100755
@@ -0,0 +1,295 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+# Script to check commits for UAPI backwards compatibility
+
+set -o errexit
+set -o pipefail
+
+print_usage() {
+ name=$(basename "$0")
+ cat << EOF
+$name - check for module parameters stability across git commits.
+
+By default, the script will check to make sure the latest commit (or current
+dirty changes) did not introduce changes when compared to HEAD^1. You can
+check against additional commit ranges with the -b and -p options.
+
+Usage: $name [-b BASE_REF] [-p PAST_REF] [-j N] [-l ERROR_LOG] [-q] [-v]
+
+Options:
+ -b BASE_REF Base git reference to use for comparison. If unspecified or empty,
+ will use any dirty changes in tree to UAPI files. If there are no
+ dirty changes, HEAD will be used.
+ -p PAST_REF Compare BASE_REF to PAST_REF (e.g. -p v6.1). If unspecified or empty,
+ will use BASE_REF^1. Must be an ancestor of BASE_REF. Only headers
+ that exist on PAST_REF will be checked for compatibility.
+ -j JOBS Number of checks to run in parallel (default: number of CPU cores).
+ -l ERROR_LOG Write error log to file (default: no error log is generated).
+ -q Quiet operation (suppress stdout, still print stderr).
+ -v Verbose operation (print more information about each header being checked).
+
+Exit codes:
+ $SUCCESS) Success
+ $FAILURE) Module param differences detected
+EOF
+}
+
+readonly SUCCESS=0
+readonly FAILURE=1
+
+# Print to stderr
+eprintf() {
+ # shellcheck disable=SC2059
+ printf "$@" >&2
+}
+
+# Check if git tree is dirty
+tree_is_dirty() {
+ ! git diff --quiet
+}
+
+file_module_params_unmodified() {
+ local file="$1"
+ local base_ref="$2"
+ local past_ref="$3"
+ local base_params_file="${TMP_DIR}/${file}.base"
+ local past_params_file="${TMP_DIR}/${file}.past"
+ local error_log="${TMP_DIR}/${file}.error"
+
+ local -r awk_cmd='/^ *module_param.*\(/,/.*\);/'
+
+ mkdir -p "$(dirname "$error_log")"
+ git show "${past_ref}:${file}" 2> /dev/null \
+ | awk "$awk_cmd" > "$past_params_file" || true
+
+ # Ignore files that don't exist at the past ref or don't have module params
+ if [ ! -s "$past_params_file" ]; then
+ return 255 # Special return code for "no-op"
+ fi
+
+ if [ -z "$base_ref" ]; then
+ awk "$awk_cmd" "${KERNEL_SRC}/${file}" \
+ > "$base_params_file" 2> /dev/null || true
+ else
+ git show "${base_ref}:${file}" 2> /dev/null \
+ | awk "$awk_cmd" > "$base_params_file" || true
+ fi
+
+ # Process the param data to come up with an associative array of param names to param data
+ # For example:
+ # module_param_call(foo, set_result, get_result, NULL, 0600);
+ #
+ # is processed into:
+ # pre_change_params[foo]="set_result,get_result,NULL,0600"
+ local -A pre_change_params
+ local param_name
+ local param_params
+ while read -r mod_param_args; do
+ param_name="$(echo "$mod_param_args" | cut -d ',' -f 1)"
+ param_params="$(echo "$mod_param_args" | cut -d ',' -f 2-)"
+
+ pre_change_params[$param_name]=$param_params
+ done < <(tr -d '\t\n ' < "$past_params_file" | tr ';' '\n' | grep -o '(.*)' | tr -d '()')
+
+ local -A post_change_params
+ while read -r mod_param_args; do
+ param_name="$(echo "$mod_param_args" | cut -d ',' -f 1)"
+ param_params="$(echo "$mod_param_args" | cut -d ',' -f 2-)"
+
+ post_change_params[$param_name]=$param_params
+ done < <(tr -d '\t\n ' < "$base_params_file" | tr ';' '\n' | grep -o '(.*)' | tr -d '()')
+
+ # Flag any module param changes that:
+ # - Remove/rename a parameter
+ # - Change the arguments of the parameter
+ local incompat_param_changes=0
+ local pre
+ local post
+ for param_name in "${!pre_change_params[@]}"; do
+ pre="${pre_change_params[$param_name]}"
+ if [ ! "${post_change_params[$param_name]+set}" ]; then
+ {
+ printf "Module parameter \"%s\" in %s removed!\n" "$param_name" "$file"
+ printf " Original args: %s\n" "$pre"
+ } > "$error_log"
+ incompat_param_changes=$((incompat_param_changes + 1))
+ continue
+ fi
+
+ post="${post_change_params[$param_name]}"
+ if [ "$pre" != "$post" ]; then
+ {
+ printf "Module parameter \"%s\" in %s changed!\n" "$param_name" "$file"
+ printf " Original args: %s\n" "$pre"
+ printf " New args: %s\n" "$post"
+ } > "$error_log"
+ incompat_param_changes=$((incompat_param_changes + 1))
+ continue
+ fi
+ done
+
+ if [ "$incompat_param_changes" -gt 0 ]; then
+ return 1
+ fi
+}
+
+run() {
+ local base_ref="$1"
+ local past_ref="$2"
+ local abi_error_log="$3"
+
+ diff_args=("$past_ref")
+ if [ -n "$base_ref" ]; then
+ diff_args+=("$base_ref")
+ fi
+
+ local -a threads=()
+ local passed=0
+ local failed=0
+ printf "Checking files between %s and %s for module parameter compatibility...\n" \
+ "$past_ref" "$base_ref"
+ while read -r modified_file; do
+ if [ "${#threads[@]}" -ge "$MAX_THREADS" ]; then
+ wait "${threads[0]}" && ret="$?" || ret="$?"
+ if [ "$ret" -eq 0 ]; then
+ passed=$((passed + 1))
+ elif [ "$ret" -eq 1 ]; then
+ failed=$((failed + 1))
+ fi
+ threads=("${threads[@]:1}")
+ fi
+
+ file_module_params_unmodified "$modified_file" "$base_ref" "$past_ref" &
+ threads+=("$!")
+ done < <(git diff --diff-filter=MCD --name-only "${diff_args[@]}" -- '*.c' '*.h')
+
+ for t in "${threads[@]}"; do
+ wait "$t" && ret="$?" || ret="$?"
+ if [ "$ret" -eq 0 ]; then
+ passed=$((passed + 1))
+ elif [ "$ret" -eq 1 ]; then
+ failed=$((failed + 1))
+ fi
+ done
+
+ total=$((passed + failed))
+ if [ "$total" -eq 0 ]; then
+ printf "No files with module parameters modified between %s and %s\n" \
+ "$past_ref" "${base_ref:-dirty tree}"
+ exit 0
+ fi
+
+ if [ -n "$abi_error_log" ]; then
+ printf 'Generated by "%s %s" from git ref %s\n\n' \
+ "$0" "$*" "$(git rev-parse HEAD)" > "$abi_error_log"
+ fi
+
+ while read -r error_file; do
+ {
+ cat "$error_file"
+ printf "\n\n"
+ } | tee -a "${abi_error_log:-/dev/null}" >&2
+ done < <(find "$TMP_DIR" -type f -name '*.error')
+
+ if [ "$failed" -gt 0 ]; then
+ eprintf "error - %d/%d files with modules parameters appear _not_ to be backwards compatible\n" \
+ "$failed" "$total"
+ if [ -n "$abi_error_log" ]; then
+ eprintf "Failure summary saved to %s\n" "$abi_error_log"
+ fi
+ else
+ printf "All %d files with module_parameters checked appear to be backwards compatible\n" \
+ "$total" "$ARCH"
+ fi
+
+ exit "$failed"
+}
+
+# Make sure the git refs we have make sense
+check_refs() {
+ if ! git rev-parse --is-inside-work-tree > /dev/null 2>&1; then
+ eprintf "error - this script requires the kernel tree to be initialized with Git\n"
+ return 1
+ fi
+
+ if ! git rev-parse --verify "$past_ref" > /dev/null 2>&1; then
+ printf 'error - invalid git reference "%s"\n' "$past_ref"
+ return 1
+ fi
+
+ if [ -n "$base_ref" ]; then
+ if ! git merge-base --is-ancestor "$past_ref" "$base_ref" > /dev/null 2>&1; then
+ printf 'error - "%s" is not an ancestor of base ref "%s"\n' "$past_ref" "$base_ref"
+ return 1
+ fi
+ if [ "$(git rev-parse "$base_ref")" = "$(git rev-parse "$past_ref")" ]; then
+ printf 'error - "%s" and "%s" are the same reference\n' "$past_ref" "$base_ref"
+ return 1
+ fi
+ fi
+}
+
+main() {
+ MAX_THREADS=$(nproc)
+ quiet="false"
+ local base_ref=""
+ while getopts "hb:p:j:l:q" opt; do
+ case $opt in
+ h)
+ print_usage
+ exit "$SUCCESS"
+ ;;
+ b)
+ base_ref="$OPTARG"
+ ;;
+ p)
+ past_ref="$OPTARG"
+ ;;
+ j)
+ MAX_THREADS="$OPTARG"
+ ;;
+ l)
+ abi_error_log="$OPTARG"
+ ;;
+ q)
+ quiet="true"
+ ;;
+ *)
+ exit "$FAIL_PREREQ"
+ esac
+ done
+
+ if [ "$quiet" = "true" ]; then
+ exec > /dev/null 2>&1
+ fi
+
+ if [ -z "$KERNEL_SRC" ]; then
+ KERNEL_SRC="$(realpath "$(dirname "$0")"/..)"
+ fi
+
+ cd "$KERNEL_SRC"
+
+ if [ -z "$base_ref" ] && ! tree_is_dirty; then
+ base_ref=HEAD
+ fi
+
+ if [ -z "$past_ref" ]; then
+ if [ -n "$base_ref" ]; then
+ past_ref="${base_ref}^1"
+ else
+ past_ref=HEAD
+ fi
+ fi
+
+ if ! check_refs; then
+ exit "$FAIL_PREREQ"
+ fi
+
+ TMP_DIR=$(mktemp -d)
+ readonly TMP_DIR
+ trap 'rm -rf "$TMP_DIR"' EXIT
+
+ run "$base_ref" "$past_ref" "$abi_error_log"
+}
+
+main "$@"