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

[Li Wang <liwang@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2963 bytes --]

On Tue, Dec 21, 2021 at 5:17 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > >     v2 --> v3
> > > >       * rename to tst_disable_oom_protection
> > > >       * support set PID as 0 to protect current process
> > >
> > > > +static void set_oom_score_adj(pid_t pid, int value)
> > > > +{
> > > > +     int val;
> > > > +     char score_path[64];
> > > > +
> > > > +     if (access("/proc/self/oom_score_adj", F_OK) == -1) {
> > > We need to check here also /proc/PID/oom_score_adj, i.e. score_path.
> > >
> >
> > Good catch, I would add a 'W_OK' checking and skip the setting with
> > a reminder message if run without root.
> >
> > how about this?
> >
> > if (access(score_path, W_OK) == -1) {
> >         tst_res(TINFO, "Warning: %s cannot be accessed for writing,
> >                 please check if test run with root user.",
> >                 score_path);
>
> Hmm, I guess that we should produce TINFO if the file does not exist and
> TWARN if we cannot write to it. Something as:
>

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.



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


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");
+
+       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.",
+                               score_path);
+                       return
+               }
                tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val,
value);
+       }
 }


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 5040 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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