All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH] save_restore: Check whether path is writable
@ 2022-10-21 15:57 Martin Doucha
  2022-10-21 20:34 ` Petr Vorel
  0 siblings, 1 reply; 9+ messages in thread
From: Martin Doucha @ 2022-10-21 15:57 UTC (permalink / raw)
  To: ltp

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.

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?
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.

 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;
 	}
 
-- 
2.37.3


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-21 15:57 [LTP] [PATCH] save_restore: Check whether path is writable Martin Doucha
@ 2022-10-21 20:34 ` Petr Vorel
  2022-10-24  7:16   ` Jan Stancek
  0 siblings, 1 reply; 9+ messages in thread
From: Petr Vorel @ 2022-10-21 20:34 UTC (permalink / raw)
  To: Martin Doucha; +Cc: ltp

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-21 20:34 ` Petr Vorel
@ 2022-10-24  7:16   ` Jan Stancek
  2022-10-25 16:13     ` Martin Doucha
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Stancek @ 2022-10-24  7:16 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

On Fri, Oct 21, 2022 at 10:35 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> 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.

Problem description makes it sound like this issue affects all 3 types
of config options. Isn't the problem affecting only optional config paths?

Having entry with "(no prefix)" or "!" in save_restore implies that
test wants to write to that path - if we TCONF on root privileges or
read/write access probably doesn't make much difference - we can't
continue.

For "?" prefix, I agree that since its optional, test should be able
to run cleanly without root privileges.

> Check whether .save_restore
> > paths are writable and handle negative result the same way as if
> > the path does not exist.

Do you mean for "?" prefix only?

>
> > 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.

For optional path, if test can't read/write it (b/o of no root privileges),
I think library shouldn't try to save it - then that would also skip
attempt to restore it.

>
> > 2) Should the '?' flag skip read-only files as if they don't exist?
I think yes. If we can't restore those files, that means test shouldn't
be able to change those and we don't need to worry about saving them.

> >    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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-24  7:16   ` Jan Stancek
@ 2022-10-25 16:13     ` Martin Doucha
  2022-10-26 11:29       ` Jan Stancek
  2022-10-26 13:08       ` Cyril Hrubis
  0 siblings, 2 replies; 9+ messages in thread
From: Martin Doucha @ 2022-10-25 16:13 UTC (permalink / raw)
  To: Jan Stancek, Petr Vorel; +Cc: ltp

On 24. 10. 22 9:16, Jan Stancek wrote:
> On Fri, Oct 21, 2022 at 10:35 PM Petr Vorel <pvorel@suse.cz> wrote:
> Problem description makes it sound like this issue affects all 3 types
> of config options. Isn't the problem affecting only optional config paths?
> 
> Having entry with "(no prefix)" or "!" in save_restore implies that
> test wants to write to that path - if we TCONF on root privileges or
> read/write access probably doesn't make much difference - we can't
> continue.
> 
> For "?" prefix, I agree that since its optional, test should be able
> to run cleanly without root privileges.

It does affect all 3 but slightly differently, depending on the "val" 
field of the respective .save_restore item. The current implementation 
behaves like this without root privileges:
- (no prefix): If val is NULL, the test will save the data, run the test 
and trigger TWARN at the end. If val is not NULL, the test will fail 
immediately after saving sysfile data because it'll try to write into a 
read-only file. We'd want TCONF instead in the latter case.
- '!': Same behavior as with no prefix but we want to keep it.
- '?': Same behavior as with no prefix. We want either TCONF or to 
ignore the sysfile entirely with a TINFO message.

> For optional path, if test can't read/write it (b/o of no root privileges),
> I think library shouldn't try to save it - then that would also skip
> attempt to restore it.

There are be two different kinds of optional paths, though:
1) paths that sometimes don't exist but must be written to if they do
2) paths that may be left alone if they exist and already contain the 
right value (otherwise TCONF)

So the question is whether I should steal the '?' prefix for type #2 and 
we'll introduce a new prefix later if needed, or whether we'll reserve 
the '?' prefix for type #1 according to current behavior and introduce 
the new prefix now.

On 21. 10. 22 22:34, Petr Vorel wrote:
> 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.

All those SAFE_TRY_FILE_PRINTF() calls are writing a constant so they 
can be eliminated by filling the second field of the .save_restore 
struct. I'll do that in the follow-up patchset when we agree how to 
implement this part.

-- 
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


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-25 16:13     ` Martin Doucha
@ 2022-10-26 11:29       ` Jan Stancek
  2022-10-26 13:10         ` Cyril Hrubis
  2022-10-26 13:27         ` Martin Doucha
  2022-10-26 13:08       ` Cyril Hrubis
  1 sibling, 2 replies; 9+ messages in thread
From: Jan Stancek @ 2022-10-26 11:29 UTC (permalink / raw)
  To: Martin Doucha; +Cc: ltp

On Tue, Oct 25, 2022 at 6:13 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> On 24. 10. 22 9:16, Jan Stancek wrote:
> > On Fri, Oct 21, 2022 at 10:35 PM Petr Vorel <pvorel@suse.cz> wrote:
> > Problem description makes it sound like this issue affects all 3 types
> > of config options. Isn't the problem affecting only optional config paths?
> >
> > Having entry with "(no prefix)" or "!" in save_restore implies that
> > test wants to write to that path - if we TCONF on root privileges or
> > read/write access probably doesn't make much difference - we can't
> > continue.
> >
> > For "?" prefix, I agree that since its optional, test should be able
> > to run cleanly without root privileges.
>
> It does affect all 3 but slightly differently, depending on the "val"
> field of the respective .save_restore item. The current implementation
> behaves like this without root privileges:
> - (no prefix): If val is NULL, the test will save the data, run the test
> and trigger TWARN at the end.

Is this real scenario? Why is test saving sysfs value, which is then
never changed?
I would expect that in this case, you could drop save_restore entirely.

> If val is not NULL, the test will fail
> immediately after saving sysfile data because it'll try to write into a
> read-only file.

> We'd want TCONF instead in the latter case.
> - '!': Same behavior as with no prefix but we want to keep it.
> - '?': Same behavior as with no prefix. We want either TCONF or to
> ignore the sysfile entirely with a TINFO message.
>
> > For optional path, if test can't read/write it (b/o of no root privileges),
> > I think library shouldn't try to save it - then that would also skip
> > attempt to restore it.
>
> There are be two different kinds of optional paths, though:
> 1) paths that sometimes don't exist but must be written to if they do
> 2) paths that may be left alone if they exist and already contain the
> right value (otherwise TCONF)
>
> So the question is whether I should steal the '?' prefix for type #2 and
> we'll introduce a new prefix later if needed, or whether we'll reserve
> the '?' prefix for type #1 according to current behavior and introduce
> the new prefix now.

The case 2) looks like it could apply to non-optional paths too. So maybe
best option would be to drop "!" and "?" prefixes and turn them into flags/enums
which can be then combined together.

"/proc/sys/kernel/pid_max", 0, val // TCONF if path doesn't exist
"/proc/sys/kernel/pid_max", SR_MUST_EXIST, val // TBROK if path doesn't exist
"/proc/sys/kernel/pid_max", SR_MAY_EXIST, val // if exists, save it
"/proc/sys/kernel/pid_max", SR_CONST_VAL, val // if already has val,
skip saving it
"/proc/sys/kernel/pid_max", SR_MAY_EXIST | SR_CONST_VAL, val // if
exists check it already has val, otherwise save it

What do you think? Would that make it easier to represent/implement all cases?

>
> On 21. 10. 22 22:34, Petr Vorel wrote:
> > 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.
>
> All those SAFE_TRY_FILE_PRINTF() calls are writing a constant so they
> can be eliminated by filling the second field of the .save_restore
> struct. I'll do that in the follow-up patchset when we agree how to
> implement this part.
>
> --
> 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
>


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-25 16:13     ` Martin Doucha
  2022-10-26 11:29       ` Jan Stancek
@ 2022-10-26 13:08       ` Cyril Hrubis
  2022-10-26 20:49         ` Petr Vorel
  1 sibling, 1 reply; 9+ messages in thread
From: Cyril Hrubis @ 2022-10-26 13:08 UTC (permalink / raw)
  To: Martin Doucha; +Cc: ltp

Hi!
> > For optional path, if test can't read/write it (b/o of no root privileges),
> > I think library shouldn't try to save it - then that would also skip
> > attempt to restore it.
> 
> There are be two different kinds of optional paths, though:
> 1) paths that sometimes don't exist but must be written to if they do
> 2) paths that may be left alone if they exist and already contain the 
> right value (otherwise TCONF)

Alternatively we can cleanup the interface, we moved from a single
string to a structure so we can add more fields, what about adding flags
that would describe one single attribute of the file instead of
clobbering several different characteristics of the file into a single
character?

We can then do something as:

struct tst_save_restore {
	const char *path;
	const char *val;
	/* the test needs the file to exist -> TCONF on missing */
	int required:1;
	/* write the value even if the file already contains it */
	int rewrite:1;
	...
};

This makes the inteface orthogonal and much easier to reason about.

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-26 11:29       ` Jan Stancek
@ 2022-10-26 13:10         ` Cyril Hrubis
  2022-10-26 13:27         ` Martin Doucha
  1 sibling, 0 replies; 9+ messages in thread
From: Cyril Hrubis @ 2022-10-26 13:10 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hi!
> The case 2) looks like it could apply to non-optional paths too. So maybe
> best option would be to drop "!" and "?" prefixes and turn them into flags/enums
> which can be then combined together.
> 
> "/proc/sys/kernel/pid_max", 0, val // TCONF if path doesn't exist
> "/proc/sys/kernel/pid_max", SR_MUST_EXIST, val // TBROK if path doesn't exist
> "/proc/sys/kernel/pid_max", SR_MAY_EXIST, val // if exists, save it
> "/proc/sys/kernel/pid_max", SR_CONST_VAL, val // if already has val,
> skip saving it
> "/proc/sys/kernel/pid_max", SR_MAY_EXIST | SR_CONST_VAL, val // if
> exists check it already has val, otherwise save it
> 
> What do you think? Would that make it easier to represent/implement all cases?

Ah Jan already proposed something similar to what I had in mind. I agree
with moving the attributes from the path to a separate field too.

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-26 11:29       ` Jan Stancek
  2022-10-26 13:10         ` Cyril Hrubis
@ 2022-10-26 13:27         ` Martin Doucha
  1 sibling, 0 replies; 9+ messages in thread
From: Martin Doucha @ 2022-10-26 13:27 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

On 26. 10. 22 13:29, Jan Stancek wrote:
> On Tue, Oct 25, 2022 at 6:13 PM Martin Doucha <mdoucha@suse.cz> wrote:
>> It does affect all 3 but slightly differently, depending on the "val"
>> field of the respective .save_restore item. The current implementation
>> behaves like this without root privileges:
>> - (no prefix): If val is NULL, the test will save the data, run the test
>> and trigger TWARN at the end.
> 
> Is this real scenario? Why is test saving sysfs value, which is then
> never changed?
> I would expect that in this case, you could drop save_restore entirely.

Some tests use simplified sysfile handling and only write a string 
constant passed in the .save_restore array. These have problem with lack 
of root privileges because the write happens in library code. Other 
tests write something non-constant in setup() or run(), these can handle 
read-only sysfiles themselves.

> The case 2) looks like it could apply to non-optional paths too. So maybe
> best option would be to drop "!" and "?" prefixes and turn them into flags/enums
> which can be then combined together.
> 
> "/proc/sys/kernel/pid_max", 0, val // TCONF if path doesn't exist
> "/proc/sys/kernel/pid_max", SR_MUST_EXIST, val // TBROK if path doesn't exist
> "/proc/sys/kernel/pid_max", SR_MAY_EXIST, val // if exists, save it
> "/proc/sys/kernel/pid_max", SR_CONST_VAL, val // if already has val,
> skip saving it
> "/proc/sys/kernel/pid_max", SR_MAY_EXIST | SR_CONST_VAL, val // if
> exists check it already has val, otherwise save it
> 
> What do you think? Would that make it easier to represent/implement all cases?

Sounds good, I'll prepare a patchset for that.

-- 
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


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] save_restore: Check whether path is writable
  2022-10-26 13:08       ` Cyril Hrubis
@ 2022-10-26 20:49         ` Petr Vorel
  0 siblings, 0 replies; 9+ messages in thread
From: Petr Vorel @ 2022-10-26 20:49 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: ltp

> Hi!
> > > For optional path, if test can't read/write it (b/o of no root privileges),
> > > I think library shouldn't try to save it - then that would also skip
> > > attempt to restore it.

> > There are be two different kinds of optional paths, though:
> > 1) paths that sometimes don't exist but must be written to if they do
> > 2) paths that may be left alone if they exist and already contain the 
> > right value (otherwise TCONF)

> Alternatively we can cleanup the interface, we moved from a single
> string to a structure so we can add more fields, what about adding flags
> that would describe one single attribute of the file instead of
> clobbering several different characteristics of the file into a single
> character?

> We can then do something as:

> struct tst_save_restore {
> 	const char *path;
> 	const char *val;
> 	/* the test needs the file to exist -> TCONF on missing */
> 	int required:1;
> 	/* write the value even if the file already contains it */
> 	int rewrite:1;
> 	...
> };

> This makes the inteface orthogonal and much easier to reason about.

Sounds reasonable approach to me.
Martin, Jan, WDYT?

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-10-26 20:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-21 15:57 [LTP] [PATCH] save_restore: Check whether path is writable Martin Doucha
2022-10-21 20:34 ` Petr Vorel
2022-10-24  7:16   ` Jan Stancek
2022-10-25 16:13     ` Martin Doucha
2022-10-26 11:29       ` Jan Stancek
2022-10-26 13:10         ` Cyril Hrubis
2022-10-26 13:27         ` Martin Doucha
2022-10-26 13:08       ` Cyril Hrubis
2022-10-26 20:49         ` Petr Vorel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.