[LTP] [PATCH] kill05: fix regression

[LTP] [PATCH] kill05: fix regression

[Cristian Greco]

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

Hi,

this should fix a regression in kill05.

Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?


Signed-off-by: Cristian Greco <cristian@regolo.cc>
---
 testcases/kernel/syscalls/kill/kill05.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/testcases/kernel/syscalls/kill/kill05.c
b/testcases/kernel/syscalls/kill/kill05.c index 0193100..ac5dc86 100644
--- a/testcases/kernel/syscalls/kill/kill05.c
+++ b/testcases/kernel/syscalls/kill/kill05.c
@@ -121,11 +121,12 @@ int main(int ac, char **av)
 		tst_brkm(TBROK, cleanup, "Fork failed");
 	else if (pid == 0)
 		do_master_child(av);
-
+  else {
 	if (waitpid(pid, &status, 0) == -1)
 		tst_resm(TBROK|TERRNO, "waitpid failed");
 	else if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
 		tst_resm(TFAIL, "child exited abnormally");
+  }
 	cleanup();
 	tst_exit();
 }
-- 
1.7.4.1


Thanks,
--
Cristian Greco
GPG key ID: 0xCF4D32E4

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

------------------------------------------------------------------------------
What You Don't Know About Data Connectivity CAN Hurt You
This paper provides an overview of data connectivity, details
its effect on application quality, and explores various alternative
solutions. http://p.sf.net/sfu/progress-d2d

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] kill05: fix regression

[Garrett Cooper]

On Fri, Mar 4, 2011 at 8:09 AM, Cristian Greco <cristian@regolo.cc> wrote:
> Hi,
>
> this should fix a regression in kill05.
>
> Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?

    Because it's a child process and child processes should _not_ use
libltp for sanity and to avoid introducing determinism, test bugs, and
for cluttering up output (remember: you have to communicate to the
user which process is actually doing the work, they're sharing file
descriptors potentially, etc).
    The proposed fix doesn't make sense though because do_master_child
always exits when the flow is executed correctly (i.e. there aren't
any early return statements from do_master_child, etc. Is it not
resuming the stack appropriately after it gets signaled? What's the
call flow exactly for the failure?
Thanks,
-Garrett

------------------------------------------------------------------------------
What You Don't Know About Data Connectivity CAN Hurt You
This paper provides an overview of data connectivity, details
its effect on application quality, and explores various alternative
solutions. http://p.sf.net/sfu/progress-d2d
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] kill05: fix regression

[Garrett Cooper]

On Fri, Mar 4, 2011 at 8:29 AM, Garrett Cooper <yanegomi@gmail.com> wrote:
> On Fri, Mar 4, 2011 at 8:09 AM, Cristian Greco <cristian@regolo.cc> wrote:
>> Hi,
>>
>> this should fix a regression in kill05.
>>
>> Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?
>
>    Because it's a child process and child processes should _not_ use
> libltp for sanity and to avoid introducing determinism, test bugs, and

s/determinism/non-determinism/

> for cluttering up output (remember: you have to communicate to the
> user which process is actually doing the work, they're sharing file
> descriptors potentially, etc).
>    The proposed fix doesn't make sense though because do_master_child
> always exits when the flow is executed correctly (i.e. there aren't
> any early return statements from do_master_child, etc. Is it not
> resuming the stack appropriately after it gets signaled? What's the
> call flow exactly for the failure?
> Thanks,
> -Garrett
>

------------------------------------------------------------------------------
What You Don't Know About Data Connectivity CAN Hurt You
This paper provides an overview of data connectivity, details
its effect on application quality, and explores various alternative
solutions. http://p.sf.net/sfu/progress-d2d
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] kill05: fix regression

[Cristian Greco]

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

On Fri, 4 Mar 2011 08:30:14 -0800
Garrett Cooper <yanegomi@gmail.com> wrote:

> On Fri, Mar 4, 2011 at 8:29 AM, Garrett Cooper <yanegomi@gmail.com> wrote:
> > On Fri, Mar 4, 2011 at 8:09 AM, Cristian Greco <cristian@regolo.cc> wrote:
> >> Hi,
> >>
> >> this should fix a regression in kill05.
> >>
> >> Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?
> >
> >    Because it's a child process and child processes should _not_ use
> > libltp for sanity and to avoid introducing determinism, test bugs, and
> 
> s/determinism/non-determinism/
> 
> > for cluttering up output (remember: you have to communicate to the
> > user which process is actually doing the work, they're sharing file
> > descriptors potentially, etc).
> >    The proposed fix doesn't make sense though because do_master_child
> > always exits when the flow is executed correctly (i.e. there aren't
> > any early return statements from do_master_child, etc. Is it not
> > resuming the stack appropriately after it gets signaled? What's the
> > call flow exactly for the failure?
> > Thanks,
> > -Garrett
> >

Hi Garrett,

you're right. In fact, the problem is in safe_getpwnam(), which does not
correctly reproduce the behavior of the deprecated my_getpwnam().
Moreover, I'm not noticing the problem on my laptop due to a race
condition in kill05, while it shows an inconsistent behavior on the box
I'm running tests on (mipsel with glibc, for the sake).

Could you please fix safe_getpwnam() with either one of the following
patches?

--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -98,13 +98,18 @@ struct passwd*
 safe_getpwnam(const char *file, const int lineno, void (*cleanup_fn)(void),
     const char *name)
 {
+       struct passwd *pwd;
        struct passwd *rval;
 
-       rval = getpwnam(name);
-       if (rval == NULL)
+       pwd = getpwnam(name);
+       if (pwd == NULL)
                tst_brkm(TBROK|TERRNO, cleanup_fn, "getpwnam failed at %s:%d",
                    file, lineno);
 
+       rval = (struct passwd *) malloc(sizeof(struct passwd));
+       memset(rval, 0, sizeof(struct passwd));
+       memcpy(rval, pwd, sizeof(struct passwd));
+
        return (rval);
 }

This second patch is a bit more invasive, using getpwnam_r() to
correctly retrieve the strings which getpwnam() will store in static
buffers (e.g. pw_name in kill05 is 'bin' for both the parent and the
child, while their UID is still different).

--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -98,9 +98,26 @@ struct passwd*
 safe_getpwnam(const char *file, const int lineno, void (*cleanup_fn)(void),
     const char *name)
 {
+       struct passwd *pwd;
        struct passwd *rval;
+       char *buf;
+       size_t bufsize;
 
-       rval = getpwnam(name);
+       bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
+       if (bufsize == -1)
+               bufsize = 16384;
+
+       buf = malloc(bufsize);
+       if (buf == NULL)
+               tst_brkm(TBROK|TERRNO, cleanup_fn, "malloc failed at %s:%d",
+                   file, lineno);
+
+       pwd = malloc(sizeof(struct passwd));
+       if (buf == NULL)
+               tst_brkm(TBROK|TERRNO, cleanup_fn, "malloc failed at %s:%d",
+                   file, lineno);
+
+       getpwnam_r(name, pwd, buf, bufsize, &rval);
        if (rval == NULL)
                tst_brkm(TBROK|TERRNO, cleanup_fn, "getpwnam failed at %s:%d",
                    file, lineno);


On the other hand, the race condition in kill05 should be fixed with
something like this (sorry, please double-check, I have not so much
experience with pthreads):

--- a/testcases/kernel/syscalls/kill/kill05.c
+++ b/testcases/kernel/syscalls/kill/kill05.c
@@ -77,6 +77,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <pthread.h>
 
 #include "test.h"
 #include "usctest.h"
@@ -101,6 +102,9 @@ extern int getipckey();
 
 #define TEST_SIG SIGKILL
 
+pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+
 int main(int ac, char **av)
 {
        char *msg;              /* message returned from parse_opts */
@@ -169,6 +173,11 @@ void do_master_child(char **av)
                        exit(1);
                }
 #else
+               pthread_mutex_lock(&mutex);
+               *flag = 1;
+               pthread_cond_signal(&cond);
+               pthread_mutex_unlock(&mutex);
+
                do_child();
 #endif
        }
@@ -177,10 +186,19 @@ void do_master_child(char **av)
                exit(1);
        }
 
+       pthread_mutex_lock(&mutex);
+       while (*flag == 0)
+               pthread_cond_wait(&cond, &mutex);
+       pthread_mutex_unlock(&mutex);
+
        TEST(kill(pid1, TEST_SIG));
 
        /* signal the child that we're done */
+       pthread_mutex_lock(&mutex);
        *flag = 1;
+       pthread_cond_signal(&cond);
+       pthread_mutex_unlock(&mutex);
+
 
        if (waitpid(pid1, &status, 0) == -1) {
                perror("waitpid failed");
@@ -206,15 +224,12 @@ void do_master_child(char **av)
 
 void do_child()
 {
-       pid_t my_pid;
-
-       my_pid = getpid();
-       while (1) {
-               if (*flag == 1)
-                       exit(0);
-               else
-                       sleep(1);
-       }
+       pthread_mutex_lock(&mutex);
+       while (*flag == 0)
+               pthread_cond_wait(&cond, &mutex);
+       pthread_mutex_unlock(&mutex);
+
+       exit(0);
 }
 
 void setup(void)


Thanks,
--
Cristian Greco
GPG key ID: 0xCF4D32E4

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

------------------------------------------------------------------------------
What You Don't Know About Data Connectivity CAN Hurt You
This paper provides an overview of data connectivity, details
its effect on application quality, and explores various alternative
solutions. http://p.sf.net/sfu/progress-d2d

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] kill05: fix regression

[Cyril Hrubis]

Hi!
> you're right. In fact, the problem is in safe_getpwnam(), which does not
> correctly reproduce the behavior of the deprecated my_getpwnam().
> Moreover, I'm not noticing the problem on my laptop due to a race
> condition in kill05, while it shows an inconsistent behavior on the box
> I'm running tests on (mipsel with glibc, for the sake).
> 
> Could you please fix safe_getpwnam() with either one of the following
> patches?

Technically you are right, but my_getpwnam() behaviour is broken too. As
the strings the structure is pointing to are statically allocated too,
this copying preserves only uid and gid. So instead of creating
crippled interface we will rather fix the test itself.

Thanks the report anyway.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network 
management toolset available today.  Delivers lowest initial 
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list