| Message ID | 20190306215859.66044-1-saravanak@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | setpriority01: Skip only PRIO_USER when unable to add test user | expand |
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.
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 <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 7, 2019, 3:00 AM Cyril Hrubis <<a href="mailto:chrubis@suse.cz">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi!<br> > - ltpuser = SAFE_GETPWNAM(username);<br> > - uid = ltpuser->pw_uid;<br> > + if (!tst_run_cmd(cmd_useradd, NULL, NULL, 1)) {<br> > + user_added = 1;<br> > + ltpuser = SAFE_GETPWNAM(username);<br> > + uid = ltpuser->pw_uid;<br> > + }<br> <br> The only thing that I don't like here is that we do not check the cause<br> of failure here. What exactly happens on android, is useradd missing<br> there completely?</blockquote></div></div><div dir="auto"><br></div><div dir="auto"></div><div dir="auto">Correct, useradd is missing.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> If so we should proceed with TCONF only if<br> tst_run_cmd() returned 255 and TBROK the test on other non-zero return<br> values.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">But is the test that's broken if useradd fails for wherever reason? Isn't it a configuration issue?</div><div dir="auto"><br></div><div dir="auto">Thanks,</div><div dir="auto">Saravana</div></div>
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 <div dir="ltr"><div dir="ltr"><div dir="ltr"></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 7, 2019 at 7:44 AM Saravana Kannan <<a href="mailto:saravanak@google.com" target="_blank">saravanak@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 7, 2019, 3:00 AM Cyril Hrubis <<a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br> > - ltpuser = SAFE_GETPWNAM(username);<br> > - uid = ltpuser->pw_uid;<br> > + if (!tst_run_cmd(cmd_useradd, NULL, NULL, 1)) {<br> > + user_added = 1;<br> > + ltpuser = SAFE_GETPWNAM(username);<br> > + uid = ltpuser->pw_uid;<br> > + }<br> <br> The only thing that I don't like here is that we do not check the cause<br> of failure here. What exactly happens on android, is useradd missing<br> there completely?</blockquote></div></div><div dir="auto"><br></div><div dir="auto"></div><div dir="auto">Correct, useradd is missing.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> If so we should proceed with TCONF only if<br> tst_run_cmd() returned 255 and TBROK the test on other non-zero return<br> values.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">But is the test that's broken if useradd fails for wherever reason? Isn't it a configuration issue?</div><div dir="auto"><br></div></div></blockquote><div><br></div><div>Looking more into the definition of TBROK, TCONF and the code before my changes:</div><div><br></div><div><div>- if (eaccess("/etc/passwd", W_OK))</div><div>- tst_brk(TCONF, "/etc/passwd is not accessible");</div><div>-</div><div>- tst_run_cmd(cmd_useradd, NULL, NULL, 0);</div><div>- user_added = 1;</div><div>-</div></div><div><br></div><div>Should an eaccess write failure have resulted in a TCONF? Shouldn't that have been a TBROK too?<br></div><div><br></div><div>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.</div><div><br></div><div>Thanks,</div><div>Saravana</div></div></div></div>
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.
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.
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 > <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 11, 2019 at 9:05 AM Cyril Hrubis <<a href="mailto:chrubis@suse.cz">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br> > > But is the test that's broken if useradd fails for wherever reason? Isn't<br> > > it a configuration issue?<br> > ><br> > ><br> > Looking more into the definition of TBROK, TCONF and the code before my<br> > changes:<br> > <br> > - if (eaccess("/etc/passwd", W_OK))<br> > - tst_brk(TCONF, "/etc/passwd is not accessible");<br> > -<br> > - tst_run_cmd(cmd_useradd, NULL, NULL, 0);<br> > - user_added = 1;<br> > -<br> > <br> > Should an eaccess write failure have resulted in a TCONF? Shouldn't that<br> > have been a TBROK too?<br> <br> Looking at the git log this seems to be handling very specific case<br> where the rootfs is read only, quite likely this test was failing on<br> some specific embedded hardware. As far as I can tell this sounds like a<br> reasonable solution to the problem.<br> <br> > I'm fine with whatever way we go, except not wanting to fail the entire<br> > test case just because useradd isn't present in android. It's still very<br> > useful to run the other setpriority tests.<br> <br> Sounds good, there is no point not to map missing useradd to TCONF, I<br> just want to avoid mapping segfaulting or otherwise faulty useradd to<br> TCONF as well.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Thanks,</div><div>Saravana</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> </blockquote></div></div>
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 > > >> <div dir="ltr">Hi Cyril,<div><br></div><div>Did you have any thoughts on the v2 patch? Want me to make any updates to it?<br><div><br></div><div>Thanks,</div><div>Saravana</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 11, 2019 at 11:23 AM Saravana Kannan <<a href="mailto:saravanak@google.com">saravanak@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 11, 2019 at 9:05 AM Cyril Hrubis <<a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br> > > But is the test that's broken if useradd fails for wherever reason? Isn't<br> > > it a configuration issue?<br> > ><br> > ><br> > Looking more into the definition of TBROK, TCONF and the code before my<br> > changes:<br> > <br> > - if (eaccess("/etc/passwd", W_OK))<br> > - tst_brk(TCONF, "/etc/passwd is not accessible");<br> > -<br> > - tst_run_cmd(cmd_useradd, NULL, NULL, 0);<br> > - user_added = 1;<br> > -<br> > <br> > Should an eaccess write failure have resulted in a TCONF? Shouldn't that<br> > have been a TBROK too?<br> <br> Looking at the git log this seems to be handling very specific case<br> where the rootfs is read only, quite likely this test was failing on<br> some specific embedded hardware. As far as I can tell this sounds like a<br> reasonable solution to the problem.<br> <br> > I'm fine with whatever way we go, except not wanting to fail the entire<br> > test case just because useradd isn't present in android. It's still very<br> > useful to run the other setpriority tests.<br> <br> Sounds good, there is no point not to map missing useradd to TCONF, I<br> just want to avoid mapping segfaulting or otherwise faulty useradd to<br> TCONF as well.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Thanks,</div><div>Saravana</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> </blockquote></div></div> </blockquote></div>
Hi! Sorry for late reply, looking into useradd manual mapping return value 1 to TCONF should do the job here.
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)
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(-)