[LTP] [PATCH] save_restore: Introduce new struct field for flags
Jan Stancek
jstancek@redhat.com
Fri Nov 11 10:47:34 CET 2022
On Sun, Nov 6, 2022 at 7:24 AM Li Wang <liwang@redhat.com> wrote:
>
>
>
> On Fri, Nov 4, 2022 at 8:07 PM Martin Doucha <mdoucha@suse.cz> wrote:
>>
>> Tests using the .save_restore functionality currently cannot run
>> without root privileges at all because the test will write
>> into the path at least at the end and trigger error, even when
>> the config paths are flagged as optional.
>>
>> Introduce new tst_path_val field for flags and replace path prefix flags
>> with bit flags. Also introduce new flags to control handling of read/write
>> errors and read-only sysfiles and rewrite save_restore implementation
>> accordingly.
>>
>> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
>> ---
>>
>> This is technically a v2 for
>> https://patchwork.ozlabs.org/project/ltp/patch/20221021155740.8339-1-mdoucha@suse.cz/
>>
>> I'll send a follow-up patchset to replace setup() code which requires root
>> privileges without good reason after this patch gets merged. Here I've kept
>> test changes to the minimum needed to maintain current save_restore behavior
>> with the new flags system. The only change in behavior is the use of read-only
>> handling flags where it's clear that the change is desired.
>>
>> Though a few tests should get closer attention during review:
>> - all KSM tests
>> - add_key05
>> - migrate_pages02
>>
>> doc/c-test-api.txt | 38 ++++----
>> include/tst_sys_conf.h | 15 ++-
>> lib/tst_sys_conf.c | 97 ++++++++++++-------
>> lib/tst_test.c | 3 +-
>> testcases/cve/icmp_rate_limit01.c | 2 +-
>> testcases/kernel/containers/userns/userns08.c | 2 +-
>> testcases/kernel/kvm/kvm_pagefault01.c | 3 +-
>> testcases/kernel/mem/ksm/ksm01.c | 10 +-
>> testcases/kernel/mem/ksm/ksm02.c | 10 +-
>> testcases/kernel/mem/ksm/ksm03.c | 10 +-
>> testcases/kernel/mem/ksm/ksm04.c | 10 +-
>> testcases/kernel/mem/ksm/ksm05.c | 2 +-
>> testcases/kernel/mem/ksm/ksm06.c | 9 +-
>> testcases/kernel/syscalls/add_key/add_key05.c | 7 +-
>> testcases/kernel/syscalls/bind/bind06.c | 2 +-
>> testcases/kernel/syscalls/fork/fork13.c | 2 +-
>> .../kernel/syscalls/ipc/msgget/msgget03.c | 2 +-
>> testcases/kernel/syscalls/madvise/madvise06.c | 2 +-
>> testcases/kernel/syscalls/madvise/madvise08.c | 2 +-
>> .../syscalls/migrate_pages/migrate_pages02.c | 2 +-
>> testcases/kernel/syscalls/sendto/sendto03.c | 2 +-
>> .../kernel/syscalls/setsockopt/setsockopt05.c | 2 +-
>> .../kernel/syscalls/setsockopt/setsockopt06.c | 2 +-
>> .../kernel/syscalls/setsockopt/setsockopt07.c | 2 +-
>> .../kernel/syscalls/setsockopt/setsockopt08.c | 2 +-
>> .../kernel/syscalls/setsockopt/setsockopt09.c | 2 +-
>> testcases/kernel/syscalls/syslog/syslog11.c | 2 +-
>> 27 files changed, 142 insertions(+), 102 deletions(-)
>>
>> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
>> index e4c66b492..dbca6266b 100644
>> --- a/doc/c-test-api.txt
>> +++ b/doc/c-test-api.txt
>> @@ -1602,35 +1602,33 @@ LTP library can be instructed to save and restore value of specified
>> field 'save_restore'. It is a NULL-terminated array of struct
>> 'tst_path_val' where each tst_path_val.path represents a file, whose
>> value is saved at the beginning and restored at the end of the test.
>> -If non-NULL value is passed it is written to the respective file at
>> -the beginning of the test. Only the first line of a specified file
>> -is saved and restored.
>> -
>> -Pathnames can be optionally prefixed to specify how strictly (during
>> -'store') are handled errors:
>> -
>> -* (no prefix) - test ends with 'TCONF', if file doesn't exist
>> -* '?' - test prints info message and continues,
>> - if file doesn't exist or open/read fails
>> -* '!' - test ends with 'TBROK', if file doesn't exist
>> +If non-NULL string is passed in tst_path_val.val, it is written
>> +to the respective file at the beginning of the test. Only the first line
>> +of a specified file is saved and restored.
>> +
>> +By default, the test will end with TCONF if the file is read-only or
>> +does not exist. If the optional write of new value fails, the test will end
>> +with 'TBROK'. This behavior can be changed using tst_path_val.flags:
>> +
>> +* 'TST_SR_FAIL_MISSING' – End test with 'TBROK' if the file does not exist
>> +* 'TST_SR_IGNORE_MISSING' – Continue without saving the file if it does not exist
>> +* 'TST_SR_FAIL_RO' – End test with 'TBROK' if the file is read-only
>> +* 'TST_SR_IGNORE_RO' – Continue without saving the file if it is read-only
>> +* 'TST_SR_IGNORE_ERR' – Ignore errors when writing new value into the file
>> +* 'TST_SR_REQUIRED' – Equivalent to 'TST_SR_FAIL_MISSING | TST_SR_FAIL_RO'
>> +* 'TST_SR_IF_ACCESS' – Equivalent to 'TST_SR_IGNORE_MISSING | TST_SR_IGNORE_RO'
>>
>> 'restore' is always strict and will TWARN if it encounters any error.
>>
>> [source,c]
>> -------------------------------------------------------------------------------
>> -static void setup(void)
>> -{
>> - FILE_PRINTF("/proc/sys/kernel/core_pattern", "/mypath");
>> - SAFE_TRY_FILE_PRINTF("/proc/sys/user/max_user_namespaces", "%d", 10);
>> -}
>> -
>> static struct tst_test test = {
>> ...
>> .setup = setup,
>> .save_restore = (const struct tst_path_val[]) {
>> - {"/proc/sys/kernel/core_pattern", NULL},
>> - {"?/proc/sys/user/max_user_namespaces", NULL},
>> - {"!/sys/kernel/mm/ksm/run", "1"},
>> + {"/proc/sys/kernel/core_pattern", NULL, 0},
>> + {"/proc/sys/user/max_user_namespaces", NULL, TST_SR_IF_ACCESS},
>> + {"/sys/kernel/mm/ksm/run", "1", TST_SR_REQUIRED},
>> {}
>> },
>> };
>> diff --git a/include/tst_sys_conf.h b/include/tst_sys_conf.h
>> index b7bbe36fc..a87f4e1cd 100644
>> --- a/include/tst_sys_conf.h
>> +++ b/include/tst_sys_conf.h
>> @@ -5,14 +5,23 @@
>> #ifndef TST_SYS_CONF_H__
>> #define TST_SYS_CONF_H__
>>
>> +#define TST_SR_FAIL_MISSING 0x1
>> +#define TST_SR_IGNORE_MISSING 0x2
>> +#define TST_SR_FAIL_RO 0x4
>> +#define TST_SR_IGNORE_RO 0x8
>> +#define TST_SR_IGNORE_ERR 0x10
>
>
> This is a good thought to split the permission and respectively
> handle them, but I feel that these names are a bit ambiguous.
>
> For example TST_SR_IGNORE_RO, I was puzzled a while when
> reading it in the below code, it can be thought of as ignoring the
> READ_ONLY permission if just from the literal meaning.
>
> We need to find more precise names.
Would TST_SR_SKIP_RO be clearer?
>
>
>>
>> +
>> +#define TST_SR_REQUIRED (TST_SR_FAIL_MISSING | TST_SR_FAIL_RO)
>> +#define TST_SR_IF_ACCESS (TST_SR_IGNORE_MISSING | TST_SR_IGNORE_RO)
>
>
> Here as well, especially these two will be more frequently used in testcase writing.
Maybe TST_SR_COND_ACCESS - which slightly longer
Anyway, moving from prefix to flags is desired change, so:
Acked-by: Jan Stancek <jstancek@redhat.com>
More information about the ltp
mailing list