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

[LTP] [PATCH] 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.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 .../kernel/syscalls/setpriority/Makefile      |  5 -----
 .../syscalls/setpriority/setpriority01.c      | 22 +++++++++++--------
 2 files changed, 13 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..147d173c5 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);
@@ -116,14 +123,11 @@ static void setup(void)
 	const char *const cmd_useradd[] = {"useradd", username, NULL};
 	struct passwd *ltpuser;
 
-	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;
+	if (!tst_run_cmd(cmd_useradd, NULL, NULL, 1)) {
+		user_added = 1;
+		ltpuser = SAFE_GETPWNAM(username);
+		uid = ltpuser->pw_uid;
+	}
 }
 
 static void cleanup(void)
-- 
2.21.0.352.gf09ad66450-goog

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

[Cyril Hrubis]

Hi!
> -	ltpuser = SAFE_GETPWNAM(username);
> -	uid = ltpuser->pw_uid;
> +	if (!tst_run_cmd(cmd_useradd, NULL, NULL, 1)) {
> +		user_added = 1;
> +		ltpuser = SAFE_GETPWNAM(username);
> +		uid = ltpuser->pw_uid;
> +	}

The only thing that I don't like here is that we do not check the cause
of failure here. What exactly happens on android, is useradd missing
there completely? If so we should proceed with TCONF only if
tst_run_cmd() returned 255 and TBROK the test on other non-zero return
values.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Saravana Kannan]

On Thu, Mar 7, 2019, 3:00 AM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > -     ltpuser = SAFE_GETPWNAM(username);
> > -     uid = ltpuser->pw_uid;
> > +     if (!tst_run_cmd(cmd_useradd, NULL, NULL, 1)) {
> > +             user_added = 1;
> > +             ltpuser = SAFE_GETPWNAM(username);
> > +             uid = ltpuser->pw_uid;
> > +     }
>
> The only thing that I don't like here is that we do not check the cause
> of failure here. What exactly happens on android, is useradd missing
> there completely?


Correct, useradd is missing.

If so we should proceed with TCONF only if
> tst_run_cmd() returned 255 and TBROK the test on other non-zero return
> values.
>

But is the test that's broken if useradd fails for wherever reason? Isn't
it a configuration issue?

Thanks,
Saravana
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190307/b2e08468/attachment.html>

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

[Saravana Kannan]

On Thu, Mar 7, 2019 at 7:44 AM Saravana Kannan <saravanak@google.com> wrote:

>
>
> On Thu, Mar 7, 2019, 3:00 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
>> Hi!
>> > -     ltpuser = SAFE_GETPWNAM(username);
>> > -     uid = ltpuser->pw_uid;
>> > +     if (!tst_run_cmd(cmd_useradd, NULL, NULL, 1)) {
>> > +             user_added = 1;
>> > +             ltpuser = SAFE_GETPWNAM(username);
>> > +             uid = ltpuser->pw_uid;
>> > +     }
>>
>> The only thing that I don't like here is that we do not check the cause
>> of failure here. What exactly happens on android, is useradd missing
>> there completely?
>
>
> Correct, useradd is missing.
>
> If so we should proceed with TCONF only if
>> tst_run_cmd() returned 255 and TBROK the test on other non-zero return
>> values.
>>
>
> But is the test that's broken if useradd fails for wherever reason? Isn't
> it a configuration issue?
>
>
Looking more into the definition of TBROK, TCONF and the code before my
changes:

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

Should an eaccess write failure have resulted in a TCONF? Shouldn't that
have been a TBROK too?

I'm fine with whatever way we go, except not wanting to fail the entire
test case just because useradd isn't present in android. It's still very
useful to run the other setpriority tests.

Thanks,
Saravana
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190307/9349e098/attachment-0001.html>

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

[Cyril Hrubis]

Hi!
> > But is the test that's broken if useradd fails for wherever reason? Isn't
> > it a configuration issue?
> >
> >
> Looking more into the definition of TBROK, TCONF and the code before my
> changes:
> 
> -       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;
> -
> 
> Should an eaccess write failure have resulted in a TCONF? Shouldn't that
> have been a TBROK too?

Looking at the git log this seems to be handling very specific case
where the rootfs is read only, quite likely this test was failing on
some specific embedded hardware. As far as I can tell this sounds like a
reasonable solution to the problem.

> I'm fine with whatever way we go, except not wanting to fail the entire
> test case just because useradd isn't present in android. It's still very
> useful to run the other setpriority tests.

Sounds good, there is no point not to map missing useradd to TCONF, I
just want to avoid mapping segfaulting or otherwise faulty useradd to
TCONF as well.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Saravana Kannan]

On Mon, Mar 11, 2019 at 9:05 AM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > But is the test that's broken if useradd fails for wherever reason?
> Isn't
> > > it a configuration issue?
> > >
> > >
> > Looking more into the definition of TBROK, TCONF and the code before my
> > changes:
> >
> > -       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;
> > -
> >
> > Should an eaccess write failure have resulted in a TCONF? Shouldn't that
> > have been a TBROK too?
>
> Looking at the git log this seems to be handling very specific case
> where the rootfs is read only, quite likely this test was failing on
> some specific embedded hardware. As far as I can tell this sounds like a
> reasonable solution to the problem.
>
> > I'm fine with whatever way we go, except not wanting to fail the entire
> > test case just because useradd isn't present in android. It's still very
> > useful to run the other setpriority tests.
>
> Sounds good, there is no point not to map missing useradd to TCONF, I
> just want to avoid mapping segfaulting or otherwise faulty useradd to
> TCONF as well.
>

Thanks for the reviews and comments! I sent out a v2 patch that addressed
your concerns. Can you please take a look at that and accept or suggest
changes to it? In that patch, I treated the "no write permissions to
/etc/passwd" as a TBROK and not TCONF as that seemed more appropriate to
me. But if you want I can map that back to TCONF -- according to man pages,
useradd has an error code for not being able to write to /etc/passwd.

Thanks,
Saravana


>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190311/22cd6b68/attachment.html>

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

[Saravana Kannan]

Hi Cyril,

Did you have any thoughts on the v2 patch? Want me to make any updates to
it?

Thanks,
Saravana

On Mon, Mar 11, 2019 at 11:23 AM Saravana Kannan <saravanak@google.com>
wrote:

>
>
> On Mon, Mar 11, 2019 at 9:05 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
>> Hi!
>> > > But is the test that's broken if useradd fails for wherever reason?
>> Isn't
>> > > it a configuration issue?
>> > >
>> > >
>> > Looking more into the definition of TBROK, TCONF and the code before my
>> > changes:
>> >
>> > -       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;
>> > -
>> >
>> > Should an eaccess write failure have resulted in a TCONF? Shouldn't that
>> > have been a TBROK too?
>>
>> Looking at the git log this seems to be handling very specific case
>> where the rootfs is read only, quite likely this test was failing on
>> some specific embedded hardware. As far as I can tell this sounds like a
>> reasonable solution to the problem.
>>
>> > I'm fine with whatever way we go, except not wanting to fail the entire
>> > test case just because useradd isn't present in android. It's still very
>> > useful to run the other setpriority tests.
>>
>> Sounds good, there is no point not to map missing useradd to TCONF, I
>> just want to avoid mapping segfaulting or otherwise faulty useradd to
>> TCONF as well.
>>
>
> Thanks for the reviews and comments! I sent out a v2 patch that addressed
> your concerns. Can you please take a look at that and accept or suggest
> changes to it? In that patch, I treated the "no write permissions to
> /etc/passwd" as a TBROK and not TCONF as that seemed more appropriate to
> me. But if you want I can map that back to TCONF -- according to man pages,
> useradd has an error code for not being able to write to /etc/passwd.
>
> Thanks,
> Saravana
>
>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190313/fa165e48/attachment.html>

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

[Cyril Hrubis]

Hi!
Sorry for late reply, looking into useradd manual mapping return value 1
to TCONF should do the job here.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Cyril Hrubis]

Hi!
> But is the test that's broken if useradd fails for wherever reason? Isn't
> it a configuration issue?

TBROK means "Test broken in preparation phase", while TCONF means "test
not applicable". Generally we use TCONF for missing functionality. If
useradd binary is present and fails to add a user for unknown reason
it's more likely failure in setting up the test environment than missing
functionality.

-- 
Cyril Hrubis
chrubis@suse.cz