Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new local_config.bash files to solve device-specific issues #1234

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Add new local_config.bash files to solve device-specific issues #1234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5948867
fix statically -> statistically typo in apause.exp
marc-hb Aug 13, 2024
ea0419b
sof-kernel-log-check.sh: simplify journalctl logic
marc-hb Sep 23, 2024
b42cc3c
lib.sh: source additional files local_config.bash
marc-hb Sep 23, 2024
33c5730
sof-kernel-log-check.sh: add new "${sof_local_extra_kernel_ignores[@]}"
marc-hb Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions case-lib/apause.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env expect

# If you're new to expect:
# If you're new to https://wiki.tcl-lang.org/page/Expect
#
# - Expect is "only" a Tcl extension and Tcl command.
# An "Expect script" is a somewhat misleading shorthand for "a Tcl
Expand Down Expand Up @@ -186,7 +186,7 @@ expect {
# for some unknown reason, then there could be _multiple_
# volume lines in a single of these buffer iterations and then we
# could miss some non-zeros.
# This is very unlikely though so this is statically good enough.
# This is very unlikely though so this is statistically good enough.
if {! [regexp {\| ( |0)0%} "$buffer_with_lf"]} {
set volume_always_zero false
}
Expand Down
4 changes: 4 additions & 0 deletions case-lib/config.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

# this file is used for defining global variables

# To change some of these variables without polluting git status/git
# diff, override them in either /etc/sof/local_config.bash or
# sof-test/case-lib/local_config.bash

# Some variables need manual configuration
# Some commands need to access system node, so they need the sudo password
SUDO_PASSWD=${SUDO_PASSWD:-}
Expand Down
9 changes: 9 additions & 0 deletions case-lib/lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ source "$SCRIPT_HOME/case-lib/pipeline.sh"
# shellcheck source=case-lib/hijack.sh
source "$SCRIPT_HOME/case-lib/hijack.sh"

# source these last (and in this order) so they win
for f in /etc/sof/local_config.bash ${SCRIPT_HOME}/case-lib/local_config.bash; do
if test -e "$f"; then
dlogw "Sourcing local and optional $f"
# shellcheck disable=SC1090
source "$f"
fi
done

# restrict bash version for some bash feature
[[ $(echo -e "$BASH_VERSION\n4.1"|sort -V|head -n 1) == '4.1' ]] || {
dlogw "Bash version: ${BASH_VERSINFO[0]}.${BASH_VERSINFO[1]} should > 4.1"
Expand Down
25 changes: 17 additions & 8 deletions tools/sof-kernel-log-check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,18 @@ ignore_str="$ignore_str"'|nvme0: Admin Cmd\(0x[[:digit:]]+\), I/O Error \(sct 0x
# SDW related logs
#

# This expects an array like for instance:
# sof_local_extra_kernel_ignores=(-e 'error 1' -e err2)
# Arrays
# - provide whitespace/quoting safety
# - can be empty/optional
# - can be arbitrarily complex. Best avoided but only on specific systems anyway.
# shellcheck disable=SC2154
if &>/dev/null declare -p sof_local_extra_kernel_ignores; then
dlogw "Ignoring extra errors on this particular system:"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern I have with this facility is that when adding local exceptions becomes easier, it can be used too much and people reading the test logs might not realize some DUTs have local exceptions others don't have. The global ignore list has drawbacks, but at least one needs to make a git commit with proper description and leave something that can be search through git history. OTOH, this dlogw does make local exception use clear in the log, which alleviates my concern. It really depends on how this will be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

This is a risk but historically, the abuse has been the other way around: the global list has been abused to add issues with "personal" devices. Just look at the git blame, log and past PRs. I expect the existence of this new feature to be missed and the existing abuse to continue :-) Also, it may seem quick to edit files on one device but what 5 or 6?

Both situations exist, I really think the code should have offered both possibilities a long time ago...

declare -p sof_local_extra_kernel_ignores
fi

# confirm begin_timestamp is in UNIX timestamp format, otherwise search full log
if [[ $begin_timestamp =~ ^[0-9]{10} ]]; then
cmd="journalctl_cmd --since=@$begin_timestamp"
Expand All @@ -427,18 +439,15 @@ else
fi

declare -p cmd
# check priority err for error message
if [[ "$ignore_str" ]]; then
err=$($cmd --priority=err | grep -vE "$ignore_str")
else
err=$($cmd --priority=err)
fi

[[ -z "$err" ]] || {
if err=$($cmd --priority=err |
grep -v -E -e "$ignore_str" "${sof_local_extra_kernel_ignores[@]}"); then

type journalctl_cmd
echo "$(date -u '+%Y-%m-%d %T %Z')" "[ERROR]" "Caught kernel log error"
echo "===========================>>"
echo "$err"
echo "<<==========================="
builtin exit 1
}

fi
Loading