On Fri, Jul 10, 2020 at 05:27:35PM +0200, Petr Mladek wrote:

On Fri 2020-07-10 10:40:43, Kamalesh Babulal wrote:

quoted

With procfs v3.3.16, the sysctl command doesn't prints the set key and
value on error.  This change breaks livepatch selftest test-ftrace.sh,
that tests the interaction of sysctl ftrace_enabled:

Good catch, it looks like it was this procps commit that modified that
behavior:

  commit da82fe49b1476d227874905068adb69577e11d96
  Author: Patrick Steinhardt [off-list ref]
  Date:   Tue May 29 13:29:03 2018 +0200
  
      sysctl: do not report set key in case `close_stream` fails
      
      As we're using buffered I/O when writing kernel parameters, write errors
      may get delayed until we close the `FILE` stream. As we are currently
      outputting the key that is to be set disregarding the return value of
      `close_stream`, we may end up in a situation where we report error and
      success:
      
          $ sysctl kernel.printk_ratelimit=100000000000000
          sysctl: setting key "kernel.printk_ratelimit": error code 22
          kernel.printk_ratelimit = 100000000000000
      
      Fix the issue by only outputting the updated value in case
      `close_stream` does not report an error.
      
      Signed-off-by: Patrick Steinhardt [off-list ref]

And I'd agree that echoing the failed new value was confusing to see
from a user's perspective.

quoted

 # selftests: livepatch: test-ftrace.sh
 # TEST: livepatch interaction with ftrace_enabled sysctl ... not ok
 #
 # --- expected
 # +++ result
 # @@ -16,7 +16,7 @@ livepatch: 'test_klp_livepatch': initial
 #  livepatch: 'test_klp_livepatch': starting patching transition
 #  livepatch: 'test_klp_livepatch': completing patching transition
 #  livepatch: 'test_klp_livepatch': patching complete
 # -livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
    resource busy kernel.ftrace_enabled = 0
 # +livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
    resource busy
 #  % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
 #  livepatch: 'test_klp_livepatch': initializing unpatching transition
 #  livepatch: 'test_klp_livepatch': starting unpatching transition
 #
 # ERROR: livepatch kselftest(s) failed

on setting sysctl kernel.ftrace_enabled={0,1} value successfully, the
set key and value is displayed.

This patch fixes it by limiting the output from both the cases to eight
words, that includes the error message or set key and value on failure
and success. The upper bound of eight words is enough to display the
only tracked error message. Also, adjust the check_result string in
test-ftrace.sh to match the expected output.

This looks really tricky.

I wonder if we could use "sysctl -q" to refuse printing the value
even with older versions. The following patch works here with
sysctl 3.3.15:

FWIW, --quiet was added to procps way back in 2004, so it should be safe
to use... and there's already a bunch of net selftests using it.

I think this method is less fragile than word count / cutting and we get
to drop that strange 'paste' invocation (I had to look that up in the
mapages to remember why we used it).

Regards,

-- Joe