All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
@ 2021-04-13 11:54 Zhao Gongyi
  2021-04-15 11:32 ` Cyril Hrubis
  0 siblings, 1 reply; 17+ messages in thread
From: Zhao Gongyi @ 2021-04-13 11:54 UTC (permalink / raw)
  To: ltp

When main process has been killed, its son process will be reaped by
init, and son process will send SIGUSR1 to the init process.

In busybox, send SIGUSR1 to the init process will trig shutdown.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
v1->v2: add exit(TBROK) after kill
---
 lib/tst_test.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 45753d1ca..446887c31 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1156,6 +1156,16 @@ static void heartbeat(void)
 	if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");

+	if (getppid() == 1) {
+		tst_res(TFAIL, "Main test process might have exit!");
+		/*
+		 * We need kill the task group immediately since the
+		 * main process has exit.
+		 */
+		kill(0, SIGKILL);
+		exit(TBROK);
+	}
+
 	kill(getppid(), SIGUSR1);
 }

--
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-13 11:54 [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat Zhao Gongyi
@ 2021-04-15 11:32 ` Cyril Hrubis
  2021-04-15 14:05   ` Cyril Hrubis
  0 siblings, 1 reply; 17+ messages in thread
From: Cyril Hrubis @ 2021-04-15 11:32 UTC (permalink / raw)
  To: ltp

Hi!
This should have had v2 in the email subject, please do not forget to
add the -v flag when generating/sending patches to git.

Otherwise this looks fine, but let's give peter chance to look at it.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-15 11:32 ` Cyril Hrubis
@ 2021-04-15 14:05   ` Cyril Hrubis
  0 siblings, 0 replies; 17+ messages in thread
From: Cyril Hrubis @ 2021-04-15 14:05 UTC (permalink / raw)
  To: ltp

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-15 11:31   ` Cyril Hrubis
@ 2021-04-15 14:03     ` Petr Vorel
  0 siblings, 0 replies; 17+ messages in thread
From: Petr Vorel @ 2021-04-15 14:03 UTC (permalink / raw)
  To: ltp

> Hi!
> There is a v2 already, can you have a look?
I'm sorry to reply to the old one.  I looked at both versions, but meant to sent
review to v2.

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-15 12:11 zhaogongyi
@ 2021-04-15 13:58 ` Petr Vorel
  0 siblings, 0 replies; 17+ messages in thread
From: Petr Vorel @ 2021-04-15 13:58 UTC (permalink / raw)
  To: ltp

Hi Gongyi, Cyril,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Hi Cyril,

> I am sorry for my fault, I will make sure that it is right before sending out.
+1, Thanks!

> Thanks so much!

> Best Regards,
> Gongyi


> > Hi!
> > This should have had v2 in the email subject, please do not forget to add
> > the -v flag when generating/sending patches to git.

> > Otherwise this looks fine, but let's give peter chance to look at it.

> > Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

> > --
> > Cyril Hrubis
> > chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
@ 2021-04-15 12:11 zhaogongyi
  2021-04-15 13:58 ` Petr Vorel
  0 siblings, 1 reply; 17+ messages in thread
From: zhaogongyi @ 2021-04-15 12:11 UTC (permalink / raw)
  To: ltp

Hi Cyril,

I am sorry for my fault, I will make sure that it is right before sending out.

Thanks so much!

Best Regards,
Gongyi

> 
> Hi!
> This should have had v2 in the email subject, please do not forget to add
> the -v flag when generating/sending patches to git.
> 
> Otherwise this looks fine, but let's give peter chance to look at it.
> 
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-15  7:52 ` Petr Vorel
@ 2021-04-15 11:31   ` Cyril Hrubis
  2021-04-15 14:03     ` Petr Vorel
  0 siblings, 1 reply; 17+ messages in thread
From: Cyril Hrubis @ 2021-04-15 11:31 UTC (permalink / raw)
  To: ltp

Hi!
There is a v2 already, can you have a look?

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-03-23 13:56 Zhao Gongyi
  2021-04-12 15:10 ` Cyril Hrubis
@ 2021-04-15  7:52 ` Petr Vorel
  2021-04-15 11:31   ` Cyril Hrubis
  1 sibling, 1 reply; 17+ messages in thread
From: Petr Vorel @ 2021-04-15  7:52 UTC (permalink / raw)
  To: ltp

Hi,

LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> When main process has been killed, its son process will be reaped by
=> nit: son/child

> init, and son process will send SIGUSR1 to the init process.

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
@ 2021-04-13 11:54 zhaogongyi
  0 siblings, 0 replies; 17+ messages in thread
From: zhaogongyi @ 2021-04-13 11:54 UTC (permalink / raw)
  To: ltp

Hi Cyril,

Thanks so much!

I have resubmit the patch according your review.

Best Regards,
Gongyi

> 
> Hi!
> > Thanks for your review!
> >
> > > > +       } else
> > > > +               kill(getppid(), SIGUSR1);
> > >
> > > No need for the else branch here now that we do exit() a the end of
> the if
> > > () block.
> > >
> >
> > Generally speaking, It seems that we also need the else branch since we
> need to send heart beat to the main process.
> 
> What I meant was that the code could be structured as:
> 
> 
> 	if (getppid() == 1) {
> 		...
> 		exit(TBROK);
> 	}
> 
> 	kill(getppid(), SIGUSR1);
> 
> Hence there is no need for the else branch.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-13 10:52 zhaogongyi
@ 2021-04-13 11:32 ` Cyril Hrubis
  0 siblings, 0 replies; 17+ messages in thread
From: Cyril Hrubis @ 2021-04-13 11:32 UTC (permalink / raw)
  To: ltp

Hi!
> Thanks for your review!
> 
> > > +       } else
> > > +               kill(getppid(), SIGUSR1);
> > 
> > No need for the else branch here now that we do exit() a the end of the if
> > () block.
> >
> 
> Generally speaking, It seems that we also need the else branch since we need to send heart beat to the main process.

What I meant was that the code could be structured as:


	if (getppid() == 1) {
		...
		exit(TBROK);
	}

	kill(getppid(), SIGUSR1);

Hence there is no need for the else branch.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
@ 2021-04-13 10:52 zhaogongyi
  2021-04-13 11:32 ` Cyril Hrubis
  0 siblings, 1 reply; 17+ messages in thread
From: zhaogongyi @ 2021-04-13 10:52 UTC (permalink / raw)
  To: ltp

Hi Cyril,

Thanks for your review!

> > +       } else
> > +               kill(getppid(), SIGUSR1);
> 
> No need for the else branch here now that we do exit() a the end of the if
> () block.
>

Generally speaking, It seems that we also need the else branch since we need to send heart beat to the main process.

Best Regards,
Gongyi

> 
> Hi!
> > According to your review, is there any problems in the patch as follows:
> >
> > diff --git a/lib/tst_test.c b/lib/tst_test.c index
> > 45753d1ca..0a096d666 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -1156,7 +1156,16 @@ static void heartbeat(void)
> >         if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
> >                 tst_res(TWARN | TERRNO, "tst_clock_gettime()
> failed");
> >
> > -       kill(getppid(), SIGUSR1);
> > +       if (getppid() == 1) {
> > +               tst_res(TFAIL, "Main test process might have exit!");
> > +               /*
> > +                * We need kill the task group immediately since the
> > +                * main process has exit.
> > +                */
> > +               kill(0, SIGKILL);
> > +               exit(-1);
> 
> Not that it matters that much, but I would do exit(TBROK);
> 
> > +       } else
> > +               kill(getppid(), SIGUSR1);
> 
> No need for the else branch here now that we do exit() a the end of the if
> () block.
> 
> Other than that it looks good.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-13  8:19 zhaogongyi
@ 2021-04-13  9:37 ` Cyril Hrubis
  0 siblings, 0 replies; 17+ messages in thread
From: Cyril Hrubis @ 2021-04-13  9:37 UTC (permalink / raw)
  To: ltp

Hi!
> According to your review, is there any problems in the patch as follows:
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 45753d1ca..0a096d666 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1156,7 +1156,16 @@ static void heartbeat(void)
>         if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
>                 tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> 
> -       kill(getppid(), SIGUSR1);
> +       if (getppid() == 1) {
> +               tst_res(TFAIL, "Main test process might have exit!");
> +               /*
> +                * We need kill the task group immediately since the
> +                * main process has exit.
> +                */
> +               kill(0, SIGKILL);
> +               exit(-1);

Not that it matters that much, but I would do exit(TBROK);

> +       } else
> +               kill(getppid(), SIGUSR1);

No need for the else branch here now that we do exit() a the end of the
if () block.

Other than that it looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
@ 2021-04-13  8:19 zhaogongyi
  2021-04-13  9:37 ` Cyril Hrubis
  0 siblings, 1 reply; 17+ messages in thread
From: zhaogongyi @ 2021-04-13  8:19 UTC (permalink / raw)
  To: ltp

Hi Cyril,

According to your review, is there any problems in the patch as follows:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 45753d1ca..0a096d666 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1156,7 +1156,16 @@ static void heartbeat(void)
        if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
                tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");

-       kill(getppid(), SIGUSR1);
+       if (getppid() == 1) {
+               tst_res(TFAIL, "Main test process might have exit!");
+               /*
+                * We need kill the task group immediately since the
+                * main process has exit.
+                */
+               kill(0, SIGKILL);
+               exit(-1);
+       } else
+               kill(getppid(), SIGUSR1);
 }

Thanks so much!

Best Regards,
Gongyi


> 
> Hi!
> > > > +		/*
> > > > +		 * We need kill the task group immediately since the
> > > > +		 * main process has exit.
> > > > +		 */
> > > > +		kill(0, SIGKILL);
> > >
> > > Shouldn't we call exit here? There is no point in continuing once we
> > > reached this point.
> > >
> >
> > Yes, There is no point in continuing once we reached this point.
> > Considering the test might have created sub processes with system,
> > posix_spawn and so on,
> >
> > do we need to kill the task group?
> 
> Yes, I think this is the right course of action, we do the same if the test
> timeouts as well.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-04-13  6:42 zhaogongyi
@ 2021-04-13  7:42 ` Cyril Hrubis
  0 siblings, 0 replies; 17+ messages in thread
From: Cyril Hrubis @ 2021-04-13  7:42 UTC (permalink / raw)
  To: ltp

Hi!
> > > +		/*
> > > +		 * We need kill the task group immediately since the
> > > +		 * main process has exit.
> > > +		 */
> > > +		kill(0, SIGKILL);
> > 
> > Shouldn't we call exit here? There is no point in continuing once we
> > reached this point.
> >
> 
> Yes, There is no point in continuing once we reached this point. Considering the test might have created sub processes with system, posix_spawn and so on,
> 
> do we need to kill the task group?

Yes, I think this is the right course of action, we do the same if the
test timeouts as well.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
@ 2021-04-13  6:42 zhaogongyi
  2021-04-13  7:42 ` Cyril Hrubis
  0 siblings, 1 reply; 17+ messages in thread
From: zhaogongyi @ 2021-04-13  6:42 UTC (permalink / raw)
  To: ltp

Hi Cyril,

Thanks for your review!

> > -	kill(getppid(), SIGUSR1);
> > +	if (getppid() == 1) {
> > +		tst_res(TFAIL, "Main test process might have exit!");
>                          ^
> 			 This should be TBROK I guess.
> > +		/*
> > +		 * We need kill the task group immediately since the
> > +		 * main process has exit.
> > +		 */
> > +		kill(0, SIGKILL);
> 
> Shouldn't we call exit here? There is no point in continuing once we
> reached this point.
>

Yes, There is no point in continuing once we reached this point. Considering the test might have created sub processes with system, posix_spawn and so on,

do we need to kill the task group?


Best Regards,
Gongyi


> 
> Hi!
> > When main process has been killed, its son process will be reaped by
> > init, and son process will send SIGUSR1 to the init process.
> >
> > In busybox, send SIGUSR1 to the init process will trigger shutdown.
> >
> > Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> > ---
> >  lib/tst_test.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/tst_test.c b/lib/tst_test.c index
> > 45753d1ca..91dfc6bf9 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -1156,7 +1156,15 @@ static void heartbeat(void)
> >  	if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
> >  		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> >
> > -	kill(getppid(), SIGUSR1);
> > +	if (getppid() == 1) {
> > +		tst_res(TFAIL, "Main test process might have exit!");
>                          ^
> 			 This should be TBROK I guess.
> > +		/*
> > +		 * We need kill the task group immediately since the
> > +		 * main process has exit.
> > +		 */
> > +		kill(0, SIGKILL);
> 
> Shouldn't we call exit here? There is no point in continuing once we
> reached this point.
> 
> > +	} else
> > +		kill(getppid(), SIGUSR1);
> >  }
> >
> >  static void testrun(void)
> > --
> > 2.17.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
  2021-03-23 13:56 Zhao Gongyi
@ 2021-04-12 15:10 ` Cyril Hrubis
  2021-04-15  7:52 ` Petr Vorel
  1 sibling, 0 replies; 17+ messages in thread
From: Cyril Hrubis @ 2021-04-12 15:10 UTC (permalink / raw)
  To: ltp

Hi!
> When main process has been killed, its son process will be reaped by
> init, and son process will send SIGUSR1 to the init process.
> 
> In busybox, send SIGUSR1 to the init process will trigger shutdown.
> 
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
>  lib/tst_test.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 45753d1ca..91dfc6bf9 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1156,7 +1156,15 @@ static void heartbeat(void)
>  	if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
>  		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> 
> -	kill(getppid(), SIGUSR1);
> +	if (getppid() == 1) {
> +		tst_res(TFAIL, "Main test process might have exit!");
                         ^
			 This should be TBROK I guess.
> +		/*
> +		 * We need kill the task group immediately since the
> +		 * main process has exit.
> +		 */
> +		kill(0, SIGKILL);

Shouldn't we call exit here? There is no point in continuing once we
reached this point.

> +	} else
> +		kill(getppid(), SIGUSR1);
>  }
> 
>  static void testrun(void)
> --
> 2.17.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat
@ 2021-03-23 13:56 Zhao Gongyi
  2021-04-12 15:10 ` Cyril Hrubis
  2021-04-15  7:52 ` Petr Vorel
  0 siblings, 2 replies; 17+ messages in thread
From: Zhao Gongyi @ 2021-03-23 13:56 UTC (permalink / raw)
  To: ltp

When main process has been killed, its son process will be reaped by
init, and son process will send SIGUSR1 to the init process.

In busybox, send SIGUSR1 to the init process will trigger shutdown.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
 lib/tst_test.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 45753d1ca..91dfc6bf9 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1156,7 +1156,15 @@ static void heartbeat(void)
 	if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");

-	kill(getppid(), SIGUSR1);
+	if (getppid() == 1) {
+		tst_res(TFAIL, "Main test process might have exit!");
+		/*
+		 * We need kill the task group immediately since the
+		 * main process has exit.
+		 */
+		kill(0, SIGKILL);
+	} else
+		kill(getppid(), SIGUSR1);
 }

 static void testrun(void)
--
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-04-15 14:05 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 11:54 [LTP] [PATCH] lib/tst_test.c: Bugfix for heartbeat Zhao Gongyi
2021-04-15 11:32 ` Cyril Hrubis
2021-04-15 14:05   ` Cyril Hrubis
  -- strict thread matches above, loose matches on Subject: below --
2021-04-15 12:11 zhaogongyi
2021-04-15 13:58 ` Petr Vorel
2021-04-13 11:54 zhaogongyi
2021-04-13 10:52 zhaogongyi
2021-04-13 11:32 ` Cyril Hrubis
2021-04-13  8:19 zhaogongyi
2021-04-13  9:37 ` Cyril Hrubis
2021-04-13  6:42 zhaogongyi
2021-04-13  7:42 ` Cyril Hrubis
2021-03-23 13:56 Zhao Gongyi
2021-04-12 15:10 ` Cyril Hrubis
2021-04-15  7:52 ` Petr Vorel
2021-04-15 11:31   ` Cyril Hrubis
2021-04-15 14:03     ` Petr Vorel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.