Re: [LTP] [PATCH v3 1/3] lib: add functions to adjust oom score

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Not exactly, if someone gives a wrong PID, that also cannot find
> the score_path. So we shouldn't skip OOM adjustment only
> with printing the TFINO.

Right, we would have to check if the /proc/$PID/ directory exists
first.

> > if (access(score_path, F_OK)) {
> >         tst_res(TINFO,
> >                 "'%s' does not exist, skipping OOM score adjustement",
> >                 score_path);
> >         return;
> > }
> >
> > if (access(score_path, W_OK)) {
> >         tst_res(TWARN, "'%s' not writeable, are you root?", score_path);
> >         return;
> > }
> >
> 
> As Petr points out, only root user can set a negative value to
> oom_score_adj,
> this W_OK is not enough for ordinary users.

Ah, right this makes it even more complex.

> Consider about situation, I'd suggest go with non-safe macros and add
> additional check in the last.
> 
> e.g.
> 
> --- a/lib/tst_memutils.c
> +++ b/lib/tst_memutils.c
> @@ -108,17 +108,21 @@ static void set_oom_score_adj(pid_t pid, int value)
>         else
>                 sprintf(score_path, "/proc/%d/oom_score_adj", pid);
> 
> -       if (access(score_path, R_OK | W_OK) == -1) {
> -               tst_res(TINFO, "Warning: %s cannot be accessed for
> reading/writing,
> -                       please check if test run with root user.",
> -                       score_path);
> -               return
> -       }
> -
> -       SAFE_FILE_PRINTF(score_path, "%d", value);
> -       SAFE_FILE_SCANF(score_path, "%d", &val);
> -       if (val != value)
> +       if (access(score_path, F_OK) == -1)
> +               tst_brk(TBROK, "%s does not exist, please check if PID is
> valid");

Maybe we should print the pid in the message here as well.

> +
> +       FILE_PRINTF(score_path, "%d", value);
> +       FILE_SCANF(score_path, "%d", &val);
> +
> +       if (val != value) {
> +               if (value < 0) {
> +                       tst_res(TINFO, "Warning: %s cannot be set to
> negative value,
> +                               please check if test run with root user.",

I would say that we TBROK here, otherwise it could be silently ignored.
Also I would shorten the message to something as:

	"'%s' cannot be set to %i, are you root?", score_path, value);

> +                               score_path);
> +                       return
> +               }
>                 tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val,
> value);
> +       }
>  }
> 
> 
> -- 
> Regards,
> Li Wang

-- 
Cyril Hrubis
chrubis@suse.cz

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