[LTP] [PATCH] save_restore: Introduce new struct field for flags
Martin Doucha
mdoucha@suse.cz
Fri Nov 4 17:36:03 CET 2022
On 04. 11. 22 11:52, Jan Stancek wrote:
> On Thu, Nov 3, 2022 at 5:45 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
> I CC-ed Li, but it looked OK to me.
>
>> - add_key05
>> - migrate_pages02
> This looks OK to me. Though I added some notes to tests that currently
> don't have
> needs_root = 1 and where this patch introduced TST_SR_IGNORE_RO.
>
>>
>> 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
>
> Maybe split this list into low/high-level flags? I would hope for most
> tests it would be
> sufficient to pick one of high-level flags defined below. There will
> likely be some
> frequently used combinations.
Splitting the list might help if more entries get added later but for
now the number of flags is fairly short so let's keep it simple.
>> +* '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'
>
> If this should work as opposite to REQUIRED, maybe 'TST_SR_IF_AVAIL'
> would fit as name better?
I thought about something like TST_SR_IF_AVAIL while writing this patch
but it sounds too much like a synonym for TST_SR_IGNORE_MISSING. Naming
it TST_SR_IF_ACCESS puts more emphasis on access permission checks.
>> -int tst_sys_conf_save_str(const char *path, const char *value);
>> -int tst_sys_conf_save(const char *path);
>> -void tst_sys_conf_set(const char *path, const char *value);
>> +void tst_sys_conf_save_str(const char *path, const char *value);
>> +int tst_sys_conf_save(const struct tst_path_val *conf);
>
> Are you planning on using return value in follow-up patch? It does appear
> to be unused after this patch. Other than that, lib changes look OK.
I have no plans to use the return value myself so feel free to change
the return type to void during merge. I've kept the old return value
just in case it'll be used for something in the future.
>> diff --git a/testcases/cve/icmp_rate_limit01.c b/testcases/cve/icmp_rate_limit01.c
>> index 1263762d2..23fd6560c 100644
>> --- a/testcases/cve/icmp_rate_limit01.c
>> +++ b/testcases/cve/icmp_rate_limit01.c
>> @@ -269,7 +269,7 @@ static struct tst_test test = {
>> NULL
>> },
>> .save_restore = (const struct tst_path_val[]) {
>> - {"?/proc/sys/user/max_user_namespaces", NULL},
>> + {"/proc/sys/user/max_user_namespaces", NULL, TST_SR_IF_ACCESS},
>
> This test currently does not have needs_root = 1. Using TST_SR_IGNORE_RO
> implies that it can work without writing to 'max_user_namespaces', which raises
> question, why not drop it from save_restore if test can work without it?
>
> In current form, SAFE_TRY_FILE_PRINTF will TBROK for unprivileged user.
> So this should probably be for now needs_root =1 and TST_SR_IGNORE_MISSING.
As I've explained right under the commit message, the setup() code for
max_user_namespaces will be the subject of my follow-up patchset. In
this patch, I'm doing only the minimum necessary API update that
preserves current behavior. TCONF on read-only sysfile is not desirable
in these network tests.
--
Martin Doucha mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
More information about the ltp
mailing list