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

[Jan Stancek]

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