Re: [LTP] [PATCH v3] save_restore: Introduce new struct field for flags

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Martin Doucha <mdoucha@suse.cz> writes:

> 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>
> Acked-by: Jan Stancek <jstancek@redhat.com>
> ---
>
> Changes since v1:
> - TST_SR_IF_ACCESS => TST_SR_COND_ACCESS
> - TST_SR_IGNORE_RO => TST_SR_SKIP_RO
> - TST_SR_IGNORE_MISSING => TST_SR_SKIP_MISSING (to match the IGNORE_RO change)
>
> Changes since v2:
> - Fixed save_restore example in C Test API docs
>
> 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             |  3 +-
>  testcases/kernel/containers/userns/userns08.c |  3 +-
>  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       |  3 +-
>  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   |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt05.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt06.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt07.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt08.c |  3 +-
>  .../kernel/syscalls/setsockopt/setsockopt09.c |  3 +-
>  testcases/kernel/syscalls/syslog/syslog11.c   |  2 +-
>  27 files changed, 151 insertions(+), 102 deletions(-)
>
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 8844d9f2f..fa7e2d3b0 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -1605,35 +1605,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_SKIP_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_SKIP_RO' – Continue without saving the file if it is read-only

Agree with Cyril; FAIL -> TBROK.

> +* '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_COND_ACCESS' – Equivalent to 'TST_SR_SKIP_MISSING | TST_SR_SKIP_RO'

It's obvious to me what required means, but not what cond_access
means. Yet they appear to be opposites. The opposite of 'required' is
'optional'. So TST_SR_COND_ACCESS -> TST_SR_OPTIONAL.

>  
>  '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_COND_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..b52b8913e 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_SKIP_MISSING 0x2
> +#define TST_SR_FAIL_RO 0x4
> +#define TST_SR_SKIP_RO 0x8
> +#define TST_SR_IGNORE_ERR 0x10
> +
> +#define TST_SR_REQUIRED (TST_SR_FAIL_MISSING | TST_SR_FAIL_RO)
> +#define TST_SR_COND_ACCESS (TST_SR_SKIP_MISSING | TST_SR_SKIP_RO)
> +
>  struct tst_path_val {
>          const char *path;
>          const char *val;
> +	unsigned int flags;
>  };
>  
> -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);
>  void tst_sys_conf_restore(int verbose);
>  void tst_sys_conf_dump(void);
>  
> diff --git a/lib/tst_sys_conf.c b/lib/tst_sys_conf.c
> index 003698825..7aa0f46bd 100644
> --- a/lib/tst_sys_conf.c
> +++ b/lib/tst_sys_conf.c
> @@ -20,6 +20,14 @@ struct tst_sys_conf {
>  
>  static struct tst_sys_conf *save_restore_data;
>  
> +static void print_error(int info_only, const char *err, const char *path)
> +{
> +	if (info_only)
> +		tst_res(TINFO, err, path);

No TERRNO in info messages?

> +	else
> +		tst_brk(TBROK | TERRNO, err, path);
> +}
> +
>  void tst_sys_conf_dump(void)
>  {
>  	struct tst_sys_conf *i;
> @@ -28,7 +36,7 @@ void tst_sys_conf_dump(void)
>  		tst_res(TINFO, "%s = %s", i->path, i->value);
>  }
>  
> -int tst_sys_conf_save_str(const char *path, const char *value)
> +void tst_sys_conf_save_str(const char *path, const char *value)
>  {
>  	struct tst_sys_conf *n = SAFE_MALLOC(sizeof(*n));
>  
> @@ -40,45 +48,43 @@ int tst_sys_conf_save_str(const char *path, const char *value)
>  
>  	n->next = save_restore_data;
>  	save_restore_data = n;
> -
> -	return 0;
>  }
>  
> -int tst_sys_conf_save(const char *path)
> +int tst_sys_conf_save(const struct tst_path_val *conf)
>  {
>  	char line[PATH_MAX];
> +	int ttype, iret;
>  	FILE *fp;
>  	void *ret;
> -	char flag;
>  
> -	if (!path)
> +	if (!conf || !conf->path)
>  		tst_brk(TBROK, "path is empty");
>  
> -	flag = path[0];
> -	if (flag  == '?' || flag == '!')
> -		path++;
> -
> -	if (access(path, F_OK) != 0) {
> -		switch (flag) {
> -		case '?':
> -			tst_res(TINFO, "Path not found: '%s'", path);
> -			break;
> -		case '!':
> -			tst_brk(TBROK|TERRNO, "Path not found: '%s'", path);
> -			break;
> -		default:
> -			tst_brk(TCONF|TERRNO, "Path not found: '%s'", path);
> +	if (access(conf->path, F_OK) != 0) {
> +		if (conf->flags & TST_SR_SKIP_MISSING) {
> +			tst_res(TINFO, "Path not found: %s", conf->path);
>

We can use TERRNO here as well.

> +			return 1;
>  		}
> -		return 1;
> +
> +		ttype = (conf->flags & TST_SR_FAIL_MISSING) ? TBROK : TCONF;
> +		tst_brk(ttype | TERRNO, "Path not found: %s", conf->path);
>  	}
>  
> -	fp = fopen(path, "r");
> -	if (fp == NULL) {
> -		if (flag == '?')
> +	if (access(conf->path, W_OK) != 0) {
> +		if (conf->flags & TST_SR_SKIP_RO) {
> +			tst_res(TINFO, "Path is not writable: %s", conf->path);

Again TERRNO.

Otherwise makes a lot of sense.

Setting to changes requested, given the other discussion as well.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp