Re: [Devel] [PATCH vz10] selftests: pstore: skip when no backend is registered

[Konstantin Khorenko <khorenko@virtuozzo.com>]

> Without a pstore backend the console, /dev/pmsg0 and every write check
> fail.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-142447
> Feature: fix selftests
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
>
> diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
> index 4509f0cc9c918..e486a5a111d8d 100755
> --- a/tools/testing/selftests/pstore/common_tests
> +++ b/tools/testing/selftests/pstore/common_tests
> @@ -55,6 +55,7 @@ operate_files() { # tested value, files, operation
>  
>  # Parameters
>  TEST_STRING_PATTERN="Testing pstore: uuid="
> +ksft_skip=4
>  UUID=`cat /proc/sys/kernel/random/uuid`
>  TOP_DIR=`absdir $0`
>  LOG_DIR=$TOP_DIR/logs/`date +%Y%m%d-%H%M%S`_${UUID}/
> @@ -81,3 +82,7 @@ prlog -e "\tcmdline=`cat /proc/cmdline`"
>  if [ $rc -ne 0 ]; then
>      exit 1
>  fi
> +if [ -z "$backend" -o "$backend" = "(null)" ]; then
> +    prlog "pstore backend is not registered, skipping"
> +    exit $ksft_skip
> +fi

The goal is right, and it is actually what this test set has claimed to do
since 2015: pstore_crash_test:10 carries the comment

      # exit if pstore backend is not registered
      . ./common_tests

but common_tests never implemented it. The existing check at
common_tests:77-79 tests cat's exit status, i.e. only whether the sysfs file
is readable, not what it contains. So the patch finally implements what the
comment promises. Upstream common_tests is identical to ours, so this is
worth sending to mainline too.

================================================
And send to mainstream in parallel, do not wait!
================================================

1) The output now contradicts itself:

      Checking pstore backend is registered ... ok
              backend=(null)
              cmdline=...
      pstore backend is not registered, skipping

Since the patch is about exactly this check, please fix its verdict instead
of appending a second check at the end of the file. Keep the backend= and
cmdline= diagnostics - they are useful in the skip case too.

2) CONFIG_PSTORE=n (and pstore built as a module but not loaded) still FAILs
instead of skipping. CONFIG_PSTORE is tristate (fs/pstore/Kconfig:3), so
/sys/module/pstore/parameters/backend may not exist at all; then cat fails,
show_result reports FAIL, rc=1, and common_tests:82-84 exits 1 before the new
check is reached. That is the most direct "feature not built" case and it
should skip too. One extra condition.

3) More important in practice: on our config the patch is not enough once a
backend does register. We have

      CONFIG_PSTORE=y
      # CONFIG_PSTORE_CONSOLE is not set
      # CONFIG_PSTORE_PMSG is not set
      CONFIG_PSTORE_RAM=m
      CONFIG_EFI_VARS_PSTORE=y
      CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE=y

while pstore_tests checks precisely the frontends we do not have: the pstore
console (pstore_tests:13) and /dev/pmsg0 (:17, :21). The test's own config
file asks for CONFIG_PSTORE_PMSG=y and CONFIG_PSTORE_CONSOLE=y.

What saves us today is only that no backend registers by default:
efi_pstore is off (pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE),
drivers/firmware/efi/efi-pstore.c:22, early return at :261), ERST registers
only if the platform provides an ERST range (drivers/acpi/apei/erst.c:1266),
and ramoops is a module that is not autoloaded. Hence backend=(null) and the
skip fires.

But on hardware with ERST, or when booted with efivars.pstore_disable=0, or
after modprobe ramoops, pstore_tests fails again - for a reason unrelated to
the backend. If the goal is a green run under any conditions, the frontend
checks need their own skips: the console check only when a pstore console is
actually registered, the pmsg checks only when /dev/pmsg0 exists. That can be
a separate patch, but it should be decided now, otherwise the task gets
closed while the test still fails on some machines.

Nits:

- [ -z "$backend" -o "$backend" = "(null)" ]: -o inside [ is obsolescent in
  POSIX; prefer [ -z "$backend" ] || [ "$backend" = "(null)" ].
- pstore_post_reboot_tests:10-11 has the canonical comment above the same
  variable ("# Kselftest framework requirement - SKIP code is 4."); worth
  repeating it. After that the duplicate ksft_skip=4 in
  pstore_post_reboot_tests can be dropped, since it sources common_tests
  (line 13) before first using the variable (line 19).

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel