Re: [PATCH v6 3/3] check-module-params: Introduce check-module-params.sh

On Sat, Oct 28, 2023 at 4:31 AM John Moon [off-list ref] 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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel