From: David Vernet <void@manifault.com>
To: Petr Mladek <pmladek@suse.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
jpoimboe@redhat.com, jikos@kernel.org, mbenes@suse.cz,
joe.lawrence@redhat.com, corbet@lwn.net, kernel-team@fb.com
Subject: Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured
Date: Thu, 10 Feb 2022 08:22:15 -0800 [thread overview]
Message-ID: <20220210162215.oupcqlcglfpeqkbp@dev0025.ash9.facebook.com> (raw)
In-Reply-To: <YgUt4+XGLOB4+0H3@alley>
On Thu, Feb 10, 2022 at 04:23:15PM +0100, Petr Mladek wrote:
> OK, what about the following change:
>
> 1. Store only the value (number) in $result
> 2. Add optional --fail parameter
I'm fine with this approach. I agree that it's probably less confusing than
"$2" == "$1". I don't think the elif check I proposed is strictly necessary
either, and in any case the way you've rewired the patch address that.
>
> Hmm, the 1st step is not needed after several iterations here ;-)
> Anyway, it should be easier to maintain it in the long term when the
> sysctl output might change. We should probably do it in a separate patch.
>
>
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 846c7ed71556..7b624d0fd7c0 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -75,9 +75,25 @@ function set_dynamic_debug() {
> }
>
> function set_ftrace_enabled() {
> - result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
> - sysctl kernel.ftrace_enabled 2>&1)
> - echo "livepatch: $result" > /dev/kmsg
> + can_fail=0
Can you make this variable a local?
> + if [[ "$1" == "--fail" ]] ; then
> + can_fail=1
> + shift
> + fi
> +
> + err=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1)
> + result=$(sysctl --values kernel.ftrace_enabled)
> +
> + if [[ "$result" != "$1" ]] ; then
> + if [[ $can_fail -eq 1 ]] ; then
> + echo "livepatch: $err" > /dev/kmsg
> + return
> + fi
> +
> + skip "failed to set kernel.ftrace_enabled = $1"
> + fi
> +
> + echo "livepatch: kernel.ftrace_enabled = $result" > /dev/kmsg
> }
>
> function cleanup() {
> diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh
> index 552e165512f4..825540a5194d 100755
> --- a/tools/testing/selftests/livepatch/test-ftrace.sh
> +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
> @@ -25,7 +25,8 @@ if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]]
> die "livepatch kselftest(s) failed"
> fi
>
> -set_ftrace_enabled 0
> +# Check that ftrace could not get disabled when a livepatch is enabled
> +set_ftrace_enabled --fail 0
> if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then
> echo -e "FAIL\n\n"
> die "livepatch kselftest(s) failed"
Patch looks great otherwise and works fine after testing on my end. Do you
want me to send it out as a v3? Or would you prefer to just apply it as is?
Assuming it's the latter, then:
Reviewed-by: David Vernet <void@manifault.com>
Thanks,
David
next prev parent reply other threads:[~2022-02-10 16:22 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-03 23:32 [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet
2022-02-04 20:30 ` Joe Lawrence
2022-02-04 20:43 ` David Vernet
2022-02-04 20:56 ` [PATCH v2] " David Vernet
2022-02-08 15:45 ` Joe Lawrence
2022-02-09 13:02 ` Petr Mladek
2022-02-09 15:49 ` David Vernet
2022-02-10 15:23 ` Petr Mladek
2022-02-10 16:22 ` David Vernet [this message]
2022-02-10 20:58 ` Joe Lawrence
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220210162215.oupcqlcglfpeqkbp@dev0025.ash9.facebook.com \
--to=void@manifault.com \
--cc=corbet@lwn.net \
--cc=jikos@kernel.org \
--cc=joe.lawrence@redhat.com \
--cc=jpoimboe@redhat.com \
--cc=kernel-team@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=pmladek@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).