Re: [LTP] [PATCH] save_restore: Check whether path is writable

[Petr Vorel <pvorel@suse.cz>]

Hi Martin,

[ Cc Jan, who implemented the original behavior ]

> 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. Check whether .save_restore
> paths are writable and handle negative result the same way as if
> the path does not exist.
Thanks for this effort!

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---

> This is the first part of sysfile handling fixes to allow running some
> tests without root privileges again. I think this is a good enough solution
> for the save_restore part but we should discuss a few open questions first:

> 1) Is it OK to fail early during sysfile save when the test would otherwise
>    run fine but throw TWARN at the end because the sysfile is read-only?
I don't think that would be a good change.

> 2) Should the '?' flag skip read-only files as if they don't exist?
>    Alternatively, we could still let the '?' flag fail trying to write
>    into read-only sysfiles and instead introduce a new flag for cases where
>    read-only file should be skipped.
Looking at files which use '?', some of them (mostly network related, I guess
written/rewritten by Martin) use SAFE_TRY_FILE_PRINTF() on
/proc/sys/user/max_user_namespaces. It looks to me these need to to skip
read-only files, i.e. define new flag with this behavior.


Kind regards,
Petr

>  doc/c-test-api.txt | 11 +++++------
>  lib/tst_sys_conf.c | 32 ++++++++++++++++++++++----------
>  2 files changed, 27 insertions(+), 16 deletions(-)

> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 64ee3397f..0f36b5a67 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -1601,13 +1601,12 @@ 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:
> +Pathnames can be optionally prefixed to specify how to handle missing or
> +read-only files:

> -* (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
> +* (no prefix) - test ends with 'TCONF'
> +* '?'         - test prints info message and continues, even on read error
> +* '!'         - test ends with 'TBROK'

>  'restore' is always strict and will TWARN if it encounters any error.

> diff --git a/lib/tst_sys_conf.c b/lib/tst_sys_conf.c
> index 003698825..1e381a249 100644
> --- a/lib/tst_sys_conf.c
> +++ b/lib/tst_sys_conf.c
> @@ -20,6 +20,22 @@ struct tst_sys_conf {

>  static struct tst_sys_conf *save_restore_data;

> +static void print_access_error(char flag, const char *err, const char *path)
> +{
> +	switch (flag) {
> +	case '?':
> +		tst_res(TINFO, "%s: '%s'", err, path);
> +		break;
> +
> +	case '!':
> +		tst_brk(TBROK|TERRNO, "%s: '%s'", err, path);
> +		break;
> +
> +	default:
> +		tst_brk(TCONF|TERRNO, "%s: '%s'", err, path);
> +	}
> +}
> +
>  void tst_sys_conf_dump(void)
>  {
>  	struct tst_sys_conf *i;
> @@ -59,16 +75,12 @@ int tst_sys_conf_save(const char *path)
>  		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);
> -		}
> +		print_access_error(flag, "Path not found", path);
> +		return 1;
> +	}
> +
> +	if (access(path, W_OK) != 0) {
> +		print_access_error(flag, "Path is not writable", path);
>  		return 1;
>  	}

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