[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Saravana Kannan]

We don't need to skip all the tests just because we are unable to add
a test user. Not having a test user only affects PRIO_USER test case.
So just skip that one and continue running the rest of the tests when
useradd is not available.

If useradd is present and it still fails, then consider it an error in
the test preparation phase.

This also allows this test case to be built and run on Android.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---

v1:
- Skip only PRIO_USER test when unable to add user for any reason.

v2:
- Skip only PRIO_USER test when useradd isn't present
- Fail entire test with TBROK for all other cases where user can't be added

I still think v1 is the right approach. But if you want TCONF for only
for cases like Android where useradd itself isn't present, then v2 would
be the right patch.

 .../kernel/syscalls/setpriority/Makefile      |  5 ----
 .../syscalls/setpriority/setpriority01.c      | 29 +++++++++++++------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/testcases/kernel/syscalls/setpriority/Makefile b/testcases/kernel/syscalls/setpriority/Makefile
index 5d00984ea..7a1a87a28 100644
--- a/testcases/kernel/syscalls/setpriority/Makefile
+++ b/testcases/kernel/syscalls/setpriority/Makefile
@@ -19,9 +19,4 @@
 top_srcdir		?= ../../../..
 
 include $(top_srcdir)/include/mk/testcases.mk
-
-ifeq ($(ANDROID), 1)
-FILTER_OUT_MAKE_TARGETS	+= setpriority01
-endif
-
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/setpriority/setpriority01.c b/testcases/kernel/syscalls/setpriority/setpriority01.c
index 38b77b77f..92b1b4ef4 100644
--- a/testcases/kernel/syscalls/setpriority/setpriority01.c
+++ b/testcases/kernel/syscalls/setpriority/setpriority01.c
@@ -92,9 +92,16 @@ static void verify_setpriority(unsigned int n)
 {
 	struct tcase *tc = &tcases[n];
 
+	if (tc->which == PRIO_USER && !user_added) {
+		tst_res(TCONF, "setpriority(%s(%d), %d, -20..19) skipped - Can't add user",
+			str_which(tc->which), tc->which, *tc->who);
+		return;
+	}
+
 	pid = SAFE_FORK();
 	if (pid == 0) {
-		SAFE_SETUID(uid);
+		if (user_added)
+			SAFE_SETUID(uid);
 		SAFE_SETPGID(0, 0);
 
 		TST_CHECKPOINT_WAKE_AND_WAIT(0);
@@ -115,15 +122,19 @@ static void setup(void)
 {
 	const char *const cmd_useradd[] = {"useradd", username, NULL};
 	struct passwd *ltpuser;
+	int rc;
 
-	if (eaccess("/etc/passwd", W_OK))
-		tst_brk(TCONF, "/etc/passwd is not accessible");
-
-	tst_run_cmd(cmd_useradd, NULL, NULL, 0);
-	user_added = 1;
-
-	ltpuser = SAFE_GETPWNAM(username);
-	uid = ltpuser->pw_uid;
+	switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
+	case 0:
+		user_added = 1;
+		ltpuser = SAFE_GETPWNAM(username);
+		uid = ltpuser->pw_uid;
+		return;
+	case 255:
+		return;
+	default:
+		tst_brk(TBROK, "Useradd failed (%d)", rc);
+	}
 }
 
 static void cleanup(void)
-- 
2.21.0.360.g471c308f928-goog

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Petr Vorel]

Hi Saravana,

> We don't need to skip all the tests just because we are unable to add
> a test user. Not having a test user only affects PRIO_USER test case.
> So just skip that one and continue running the rest of the tests when
> useradd is not available.

> If useradd is present and it still fails, then consider it an error in
> the test preparation phase.

> This also allows this test case to be built and run on Android.

> Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> v2:
> - Skip only PRIO_USER test when useradd isn't present
> - Fail entire test with TBROK for all other cases where user can't be added

> I still think v1 is the right approach. But if you want TCONF for only
> for cases like Android where useradd itself isn't present, then v2 would
> be the right patch.
Agree with it, but waiting for Cyril approval to merge it.

Kind regards,
Petr

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Cyril Hrubis]

Hi!
> > v2:
> > - Skip only PRIO_USER test when useradd isn't present
> > - Fail entire test with TBROK for all other cases where user can't be added
> 
> > I still think v1 is the right approach. But if you want TCONF for only
> > for cases like Android where useradd itself isn't present, then v2 would
> > be the right patch.
> Agree with it, but waiting for Cyril approval to merge it.

As far as I can tell this patch still removes the read-only rootfs check
introduced in:

commit 8cab24427ec4584389cd0462eb87bd9d43261861
Author: the_hoang0709@yahoo.com <the_hoang0709@yahoo.com>
Date:   Sun Apr 9 15:38:05 2017 +0700

    setpriority01: Fix failure on read-only machine

    Return 32 if /etc/passwd is read-only.
    Fixes: #137
    See also: #88


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Petr Vorel]

Hi Cyril,

> > > I still think v1 is the right approach. But if you want TCONF for only
> > > for cases like Android where useradd itself isn't present, then v2 would
> > > be the right patch.
> > Agree with it, but waiting for Cyril approval to merge it.

> As far as I can tell this patch still removes the read-only rootfs check
> introduced in:
Good catch, eaccess check should stay in.
I'll put it back again and push, ok?

Kind regards,
Petr

> commit 8cab24427ec4584389cd0462eb87bd9d43261861
> Author: the_hoang0709@yahoo.com <the_hoang0709@yahoo.com>
> Date:   Sun Apr 9 15:38:05 2017 +0700

>     setpriority01: Fix failure on read-only machine

>     Return 32 if /etc/passwd is read-only.
>     Fixes: #137
>     See also: #88

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Cyril Hrubis]

Hi!
> > > > I still think v1 is the right approach. But if you want TCONF for only
> > > > for cases like Android where useradd itself isn't present, then v2 would
> > > > be the right patch.
> > > Agree with it, but waiting for Cyril approval to merge it.
> 
> > As far as I can tell this patch still removes the read-only rootfs check
> > introduced in:
> Good catch, eaccess check should stay in.
> I'll put it back again and push, ok?

Or we can check the return value 1 from useradd and map it to TCONF as
suggested by Saravana.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Petr Vorel]

Hi,

> > > > > I still think v1 is the right approach. But if you want TCONF for only
> > > > > for cases like Android where useradd itself isn't present, then v2 would
> > > > > be the right patch.
> > > > Agree with it, but waiting for Cyril approval to merge it.

> > > As far as I can tell this patch still removes the read-only rootfs check
> > > introduced in:
> > Good catch, eaccess check should stay in.
> > I'll put it back again and push, ok?

> Or we can check the return value 1 from useradd and map it to TCONF as
> suggested by Saravana.
Probably. According to useradd sources, exit 1 (E_PW_UPDATE) also happen when
problems to lock /etc/passwd, which could theoretically on different
circumstance than read-only /, but I guess we can ignore it.

Or we could use statvfs to detect read-only / ("can't update password file" does
can be for different purposes), but I wouldn't bother and user return value 1.

Kind regards,
Petr

[1] https://github.com/shadow-maint/shadow/blob/master/src/useradd.c#L1626

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Saravana Kannan]

The problem with using eaccess is that it doesn't build on Android as
eaccess isn't implemented.

I vote for mapping useradd exit code 1 to TCONF. Sure there are some very
unlikely cases where useradd might fail with 1 when the file system is
writable. Excluding that corner case if it means being able to run on a lot
of Android devices seems like a good compromise.

-Saravana


On Tue, Mar 19, 2019 at 4:28 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi,
>
> > > > > > I still think v1 is the right approach. But if you want TCONF
> for only
> > > > > > for cases like Android where useradd itself isn't present, then
> v2 would
> > > > > > be the right patch.
> > > > > Agree with it, but waiting for Cyril approval to merge it.
>
> > > > As far as I can tell this patch still removes the read-only rootfs
> check
> > > > introduced in:
> > > Good catch, eaccess check should stay in.
> > > I'll put it back again and push, ok?
>
> > Or we can check the return value 1 from useradd and map it to TCONF as
> > suggested by Saravana.
> Probably. According to useradd sources, exit 1 (E_PW_UPDATE) also happen
> when
> problems to lock /etc/passwd, which could theoretically on different
> circumstance than read-only /, but I guess we can ignore it.
>
> Or we could use statvfs to detect read-only / ("can't update password
> file" does
> can be for different purposes), but I wouldn't bother and user return
> value 1.
>
> Kind regards,
> Petr
>
> [1] https://github.com/shadow-maint/shadow/blob/master/src/useradd.c#L1626
>
> --
> You received this message because you are subscribed to the Google Groups
> "kernel-team" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscribe@android.com.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190319/ecb38559/attachment-0001.html>

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Saravana Kannan]

We don't need to skip all the tests just because we are unable to add
a test user. Not having a test user only affects PRIO_USER test case.
So just skip that one and continue running the rest of the tests.

This also allows this test case to be built and run on Android.

Change-Id: Ie6eaf771002842fc1c768adf70bb2d1ef534f120
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 .../kernel/syscalls/setpriority/Makefile      |  5 ----
 .../syscalls/setpriority/setpriority01.c      | 29 +++++++++++++------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/testcases/kernel/syscalls/setpriority/Makefile b/testcases/kernel/syscalls/setpriority/Makefile
index 5d00984ea..7a1a87a28 100644
--- a/testcases/kernel/syscalls/setpriority/Makefile
+++ b/testcases/kernel/syscalls/setpriority/Makefile
@@ -19,9 +19,4 @@
 top_srcdir		?= ../../../..
 
 include $(top_srcdir)/include/mk/testcases.mk
-
-ifeq ($(ANDROID), 1)
-FILTER_OUT_MAKE_TARGETS	+= setpriority01
-endif
-
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/setpriority/setpriority01.c b/testcases/kernel/syscalls/setpriority/setpriority01.c
index 38b77b77f..92b1b4ef4 100644
--- a/testcases/kernel/syscalls/setpriority/setpriority01.c
+++ b/testcases/kernel/syscalls/setpriority/setpriority01.c
@@ -92,9 +92,16 @@ static void verify_setpriority(unsigned int n)
 {
 	struct tcase *tc = &tcases[n];
 
+	if (tc->which == PRIO_USER && !user_added) {
+		tst_res(TCONF, "setpriority(%s(%d), %d, -20..19) skipped - Can't add user",
+			str_which(tc->which), tc->which, *tc->who);
+		return;
+	}
+
 	pid = SAFE_FORK();
 	if (pid == 0) {
-		SAFE_SETUID(uid);
+		if (user_added)
+			SAFE_SETUID(uid);
 		SAFE_SETPGID(0, 0);
 
 		TST_CHECKPOINT_WAKE_AND_WAIT(0);
@@ -115,15 +122,19 @@ static void setup(void)
 {
 	const char *const cmd_useradd[] = {"useradd", username, NULL};
 	struct passwd *ltpuser;
+	int rc;
 
-	if (eaccess("/etc/passwd", W_OK))
-		tst_brk(TCONF, "/etc/passwd is not accessible");
-
-	tst_run_cmd(cmd_useradd, NULL, NULL, 0);
-	user_added = 1;
-
-	ltpuser = SAFE_GETPWNAM(username);
-	uid = ltpuser->pw_uid;
+	switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
+	case 0:
+		user_added = 1;
+		ltpuser = SAFE_GETPWNAM(username);
+		uid = ltpuser->pw_uid;
+		return;
+	case 255:
+		return;
+	default:
+		tst_brk(TBROK, "Useradd failed (%d)", rc);
+	}
 }
 
 static void cleanup(void)
-- 
2.21.0.360.g471c308f928-goog

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Saravana Kannan]

Sent the wrong patch by mistake. Sending the right one (with minor edits to
commit text).

On Thu, Mar 7, 2019 at 3:28 PM Saravana Kannan <saravanak@google.com> wrote:

> We don't need to skip all the tests just because we are unable to add
> a test user. Not having a test user only affects PRIO_USER test case.
> So just skip that one and continue running the rest of the tests.
>
> This also allows this test case to be built and run on Android.
>
> Change-Id: Ie6eaf771002842fc1c768adf70bb2d1ef534f120
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  .../kernel/syscalls/setpriority/Makefile      |  5 ----
>  .../syscalls/setpriority/setpriority01.c      | 29 +++++++++++++------
>  2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/setpriority/Makefile
> b/testcases/kernel/syscalls/setpriority/Makefile
> index 5d00984ea..7a1a87a28 100644
> --- a/testcases/kernel/syscalls/setpriority/Makefile
> +++ b/testcases/kernel/syscalls/setpriority/Makefile
> @@ -19,9 +19,4 @@
>  top_srcdir             ?= ../../../..
>
>  include $(top_srcdir)/include/mk/testcases.mk
> -
> -ifeq ($(ANDROID), 1)
> -FILTER_OUT_MAKE_TARGETS        += setpriority01
> -endif
> -
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/setpriority/setpriority01.c
> b/testcases/kernel/syscalls/setpriority/setpriority01.c
> index 38b77b77f..92b1b4ef4 100644
> --- a/testcases/kernel/syscalls/setpriority/setpriority01.c
> +++ b/testcases/kernel/syscalls/setpriority/setpriority01.c
> @@ -92,9 +92,16 @@ static void verify_setpriority(unsigned int n)
>  {
>         struct tcase *tc = &tcases[n];
>
> +       if (tc->which == PRIO_USER && !user_added) {
> +               tst_res(TCONF, "setpriority(%s(%d), %d, -20..19) skipped -
> Can't add user",
> +                       str_which(tc->which), tc->which, *tc->who);
> +               return;
> +       }
> +
>         pid = SAFE_FORK();
>         if (pid == 0) {
> -               SAFE_SETUID(uid);
> +               if (user_added)
> +                       SAFE_SETUID(uid);
>                 SAFE_SETPGID(0, 0);
>
>                 TST_CHECKPOINT_WAKE_AND_WAIT(0);
> @@ -115,15 +122,19 @@ static void setup(void)
>  {
>         const char *const cmd_useradd[] = {"useradd", username, NULL};
>         struct passwd *ltpuser;
> +       int rc;
>
> -       if (eaccess("/etc/passwd", W_OK))
> -               tst_brk(TCONF, "/etc/passwd is not accessible");
> -
> -       tst_run_cmd(cmd_useradd, NULL, NULL, 0);
> -       user_added = 1;
> -
> -       ltpuser = SAFE_GETPWNAM(username);
> -       uid = ltpuser->pw_uid;
> +       switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
> +       case 0:
> +               user_added = 1;
> +               ltpuser = SAFE_GETPWNAM(username);
> +               uid = ltpuser->pw_uid;
> +               return;
> +       case 255:
> +               return;
> +       default:
> +               tst_brk(TBROK, "Useradd failed (%d)", rc);
> +       }
>  }
>
>  static void cleanup(void)
> --
> 2.21.0.360.g471c308f928-goog
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190307/2746ea65/attachment.html>

[LTP] [PATCH v2] setpriority01: Skip only PRIO_USER when unable to add test user

[Petr Vorel]

 Hi,

 FYI: Closing this one in patchwork as superseded (there is another v2).

Kind regards,
Petr