-
Notifications
You must be signed in to change notification settings - Fork 51
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.
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
Stop ignoring various logger failures #373
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need any check if logger is not running before exiting?
This looks wired if test pass but logger failed and whole test case will exit with failure.
case-lib/hijack.sh
Outdated
# logfile is set in a different file: lib.sh | ||
# shellcheck disable=SC2154 | ||
wcLog=$(wc -l "$logfile") | ||
dlogi "nlines=$wcLog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a more readable log here? This looks like a cmd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the next PR I will implement a sanity check on the number of lines. For now this is not great looking but it's readable and I'd rather not write and test code that will change in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @xiulipan !
Do we need any check if logger is not running before exiting? This looks wired if test pass but logger failed and whole test case will exit with failure.
I don't understand this comment sorry. We already had for a few weeks the "sof-logger was already dead" check that makes the test fail if the logger crashed. It works; I tested it before and after this PR.
case-lib/hijack.sh
Outdated
# logfile is set in a different file: lib.sh | ||
# shellcheck disable=SC2154 | ||
wcLog=$(wc -l "$logfile") | ||
dlogi "nlines=$wcLog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the next PR I will implement a sanity check on the number of lines. For now this is not great looking but it's readable and I'd rather not write and test code that will change in the next PR.
3d6bf81
to
2af4f11
Compare
Incomplete fix for thesofproject#298: helps only when logger is not found or not executable. Print a warning and abort tests if they requested logging _and_ they use set -e _and_ they don't explicitly ignore the error. Also get rid of $loggerBin variable that made the logic more complicated, now use $SOFLOGGER only. Signed-off-by: Marc Herbert <[email protected]>
Likely to avoid the shell printing a "Killed logger" message, lib.sh was starting the logger with a "2>/dev/null" redirection. Here's a sample list of logger errors that have been discarded by this redirection since forever: error: Unable to open ldc file /etc/sof/sof-apl.ldc error: Unable to open in file /sys/kernel/debug/sof/trace error: Unable to open version file /sys/kernel/debug/sof/fw_version Remove this 2>/dev/null redirection which was an insane price to pay for making the logs "beautiful". While not important, let's make the logs "beautiful" anyway because we can: thanks to -SIGINT instead of SIGKILL. SIGKILL (-9) does not give a any chance for the logger to exit cleanly, flus outputs etc. and is only ever used as a last resort, see "man systemd.kill" etc. Also log the logger command complete with parameters and the size of its output at the end. Signed-off-by: Marc Herbert <[email protected]>
2af4f11
to
0e78de4
Compare
@marc-hb Eve- though we had such failures in past, I want to start a discussion here: it that behavior right? Do we need to fail the case if we could not get the logger? As we have standalone test case for logger. |
The name of issue #297 is "Check for a valid trace for ALL tests" so I moved your question there. |
yes we want to fail if the logger is gone during a test. There could be something with a specific test that gets in the way of the DMA trace, and we want to trap this error. |
2 commits, see commit messages.
Green failures episode "the logger"
Incomplete fix for #297 , more coming.