All of lore.kernel.org
 help / color / mirror / Atom feed
From: Cyril Hrubis <chrubis@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] [PATCH 2/3] syscalls/getrusage: Cleanup and bugfix for getrusage03
Date: Wed, 16 Jun 2021 13:47:49 +0200	[thread overview]
Message-ID: <YMnk5QUhwnOHrWLi@yuki> (raw)
In-Reply-To: <20210616093606.214856-3-xieziyao@huawei.com>

Hi!
> diff --git a/testcases/kernel/syscalls/getrusage/getrusage03.c b/testcases/kernel/syscalls/getrusage/getrusage03.c
> index 125acfc64..518e8510e 100644
> --- a/testcases/kernel/syscalls/getrusage/getrusage03.c
> +++ b/testcases/kernel/syscalls/getrusage/getrusage03.c
> @@ -1,353 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - * getrusage03 - test ru_maxrss behaviors in struct rusage
> - *
> - * This test program is backported from upstream commit:
> - * 1f10206cf8e945220f7220a809d8bfc15c21f9a5, which fills ru_maxrss
> - * value in struct rusage according to rss hiwater mark. To make sure
> - * this feature works correctly, a series of tests are executed in
> - * this program.
> - *
>   * Copyright (C) 2011  Red Hat, Inc.
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of version 2 of the GNU General Public
> - * License as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + * Copyright (c) 2021 Xie Ziyao <xieziyao@huawei.com>
> + */
> +
> +/*\
> + * [Description]
>   *
> - * Further, this software is distributed without any warranty that it
> - * is free of the rightful claim of any third person regarding
> - * infringement or the like.  Any license provided herein, whether
> - * implied or otherwise, applies only to this software file.  Patent
> - * licenses, if any, provided herein do not apply to combinations of
> - * this program with other software, or any other product whatsoever.
> + * Test ru_maxrss behaviors in struct rusage.
>   *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> - * 02110-1301, USA.
> + * This test program is backported from upstream commit: 1f10206cf8e9, which
> + * fills ru_maxrss value in struct rusage according to rss hiwater mark. To
> + * make sure this feature works correctly, a series of tests are executed in
> + * this program.
>   */
> -#include <sys/types.h>
> -#include <sys/mman.h>
> -#include <sys/resource.h>
> -#include <sys/time.h>
> -#include <sys/wait.h>
> -#include <unistd.h>
> -#include <signal.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> 
> -#include "test.h"
> -#include "safe_macros.h"
> +#include <stdlib.h>
> +#include <stdio.h>
> 
> -char *TCID = "getrusage03";
> -int TST_TOTAL = 1;
> +#include "tst_test.h"
> +#include "getrusage03.h"
> 
> -#define DELTA_MAX	10240
> +#define TESTBIN "getrusage03_child"
> 
> +static pid_t pid;
> +static int retval;
>  static struct rusage ru;
>  static long maxrss_init;
> -static int retval, status;
> -static pid_t pid;
> 
> -static void inherit_fork(void);
> -static void inherit_fork2(void);
> -static void fork_malloc(void);
> -static void grandchild_maxrss(void);
> -static void zombie(void);
> -static void sig_ign(void);
> -static void exec_without_fork(void);
> -static void check_return(int status, char *pass_msg, char *fail_msg);
> -static int is_in_delta(long value);
> -static void consume(int mega);
> -static void setup(void);
> -static void cleanup(void);
> +static const char *const resource[] = {
> +	TESTBIN,
> +	NULL,
> +};
> 
> -int main(int argc, char *argv[])
> +static void check_result(int result, char *msg)
>  {
> -	int lc;
> -
> -	tst_parse_opts(argc, argv, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -
> -		tst_resm(TINFO, "allocate 100MB");
> -		consume(100);
> -
> -		inherit_fork();
> -		inherit_fork2();
> -		fork_malloc();
> -		grandchild_maxrss();
> -		zombie();
> -		sig_ign();
> -		exec_without_fork();
> -	}
> -	cleanup();
> -	tst_exit();
> +	if (result == 1)
> +		tst_res(TPASS, "%s", msg);
> +	else if (result == 0)
> +		tst_res(TFAIL, "%s", msg);
> +	else
> +		tst_res(TFAIL, "unexpected result???%d", result);
>  }

The new test library allows you to report PASS/FAIL from child processes
so we can get rid of this check result function here.

> -/* Testcase #01: fork inherit
> - * expect: initial.self ~= child.self */
> -static void inherit_fork(void)
> +static void inherit_fork1(void)
>  {
> -	tst_resm(TINFO, "Testcase #01: fork inherit");
> -
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_SELF, &ru);
> -	tst_resm(TINFO, "initial.self = %ld", ru.ru_maxrss);
> +	SAFE_GETRUSAGE(RUSAGE_SELF, &ru);
> +	maxrss_init = ru.ru_maxrss;
> 
> -	switch (pid = fork()) {
> -	case -1:
> -		tst_brkm(TBROK | TERRNO, cleanup, "fork #1");
> -	case 0:
> -		maxrss_init = ru.ru_maxrss;
> -		SAFE_GETRUSAGE(cleanup, RUSAGE_SELF, &ru);
> -		tst_resm(TINFO, "child.self = %ld", ru.ru_maxrss);
> -		exit(is_in_delta(maxrss_init - ru.ru_maxrss));
> -	default:
> -		break;
> +	if ((pid = SAFE_FORK()) == 0) {

We do not use the pid value for anything so this can be just:

	if (!SAFE_FORK()) {
		...

> +		SAFE_GETRUSAGE(RUSAGE_SELF, &ru);
> +		exit(!is_in_delta(maxrss_init - ru.ru_maxrss));

Here we should do:

		if (is_in_delta(maxrss_init - ru.ru_maxrss)) {
			tst_res(TPASS, "ru_maxrrs inheritted correctly");
		} else {
			tst_res(TFAIL, "ru_maxrss = %li, expected %li",
			        ru.ru_maxrss, maxrss_init);
		}

>  	}
> 
> -	SAFE_WAITPID(cleanup, pid, &status, WUNTRACED | WCONTINUED);
> -	check_return(WEXITSTATUS(status), "initial.self ~= child.self",
> -		     "initial.self !~= child.self");
> +	tst_reap_children();
> +	tst_res(TPASS, "check that initial.self ~= child.self");

And all that has to be done here is tst_reap_children();

>  }
> 
> -/* Testcase #02: fork inherit (cont.)
> - * expect: initial.children ~= 100MB, child.children = 0 */
>  static void inherit_fork2(void)
>  {
> -	tst_resm(TINFO, "Testcase #02: fork inherit(cont.)");
> -
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "initial.children = %ld", ru.ru_maxrss);
> -	if (is_in_delta(ru.ru_maxrss - 102400))
> -		tst_resm(TPASS, "initial.children ~= 100MB");
> -	else
> -		tst_resm(TFAIL, "initial.children !~= 100MB");
> -
> -	switch (pid = fork()) {
> -	case -1:
> -		tst_brkm(TBROK | TERRNO, cleanup, "fork #2");
> -	case 0:
> -		SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -		tst_resm(TINFO, "child.children = %ld", ru.ru_maxrss);
> -		exit(ru.ru_maxrss == 0);
> -	default:
> -		break;
> -	}
> -
> -	SAFE_WAITPID(cleanup, pid, &status, WUNTRACED | WCONTINUED);
> -	check_return(WEXITSTATUS(status), "child.children == 0",
> -		     "child.children != 0");
> -}
> -
> -/* Testcase #03: fork + malloc
> - * expect: initial.self + 50MB ~= child.self */
> -static void fork_malloc(void)
> -{
> -	tst_resm(TINFO, "Testcase #03: fork + malloc");
> -
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_SELF, &ru);
> -	tst_resm(TINFO, "initial.self = %ld", ru.ru_maxrss);
> +	SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +	check_result(is_in_delta(ru.ru_maxrss - 102400), "check that initial.children ~= 100MB");

Can we just do the check here instead? I.e.

	if (is_in_delta(ru.ru_maxrss - 102400))
		tst_res(TPASS, "Initial ru.ru_maxrss ~= 100MB");
	else
		tst_res(TFAIL, "Initial ru.ru_maxrss = %liB", ru.ru_maxrss);

> -	switch (pid = fork()) {
> -	case -1:
> -		tst_brkm(TBROK | TERRNO, cleanup, "fork #3");
> -	case 0:
> -		maxrss_init = ru.ru_maxrss;
> -		tst_resm(TINFO, "child allocate +50MB");
> -		consume(50);
> -		SAFE_GETRUSAGE(cleanup, RUSAGE_SELF, &ru);
> -		tst_resm(TINFO, "child.self = %ld", ru.ru_maxrss);
> -		exit(is_in_delta(maxrss_init + 51200 - ru.ru_maxrss));
> -	default:
> -		break;
> +	if ((pid = SAFE_FORK()) == 0) {

Here as well just if (!SAFE_FORK()) {

> +		SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +		exit(ru.ru_maxrss);

Here as well, we should report the result here.

>  	}
> 
> -	SAFE_WAITPID(cleanup, pid, &status, WUNTRACED | WCONTINUED);
> -	check_return(WEXITSTATUS(status), "initial.self + 50MB ~= child.self",
> -		     "initial.self + 50MB !~= child.self");
> +	tst_reap_children();
> +	tst_res(TPASS, "check that child.children == 0");

And only reap the child process here.

>  }
> 
> -/* Testcase #04: grandchild maxrss
> - * expect: post_wait.children ~= 300MB */
>  static void grandchild_maxrss(void)
>  {
> -	tst_resm(TINFO, "Testcase #04: grandchild maxrss");
> -
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "initial.children = %ld", ru.ru_maxrss);
> -
> -	switch (pid = fork()) {
> -	case -1:
> -		tst_brkm(TBROK | TERRNO, cleanup, "fork #4");
> -	case 0:
> -		retval = system("getrusage03_child -g 300");
> +	if ((pid = SAFE_FORK()) == 0) {

	Here as well.

> +		retval = tst_system("getrusage03_child grand_consume 300");

We have already forked so there is no point in using system(). All that
we have to do here is execve() as:

		const char *argv[] = {"getrusage03_child", "grand_consume", "300", NULL};

		execve(argv[0], argv[], environ);

		tst_brk(TBROK | TERRNO, "execve() failed");

>  		if ((WIFEXITED(retval) && WEXITSTATUS(retval) != 0))
> -			tst_brkm(TBROK | TERRNO, cleanup, "system");
> +			tst_brk(TBROK, "system(\"getrusage03_child grand_consume 300\")");
>  		exit(0);
> -	default:
> -		break;
>  	}
> 
> -	SAFE_WAITPID(cleanup, pid, &status, WUNTRACED | WCONTINUED);
> -	if (WEXITSTATUS(status) != 0)
> -		tst_brkm(TBROK | TERRNO, cleanup, "child exit status is not 0");
> -
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "post_wait.children = %ld", ru.ru_maxrss);
> -	if (is_in_delta(ru.ru_maxrss - 307200))
> -		tst_resm(TPASS, "child.children ~= 300MB");
> -	else
> -		tst_resm(TFAIL, "child.children !~= 300MB");
> +	tst_reap_children();
> +	SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +	check_result(is_in_delta(ru.ru_maxrss - 307200), "check that child.children ~= 300MB");
>  }
> 
> -/* Testcase #05: zombie
> - * expect: initial ~= pre_wait, post_wait ~= 400MB */
>  static void zombie(void)
>  {
> -	tst_resm(TINFO, "Testcase #05: zombie");
> -
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "initial.children = %ld", ru.ru_maxrss);
> +	SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
>  	maxrss_init = ru.ru_maxrss;
> 
> -	switch (pid = fork()) {
> -	case -1:
> -		tst_brkm(TBROK, cleanup, "fork #5");
> -	case 0:
> -		retval = system("getrusage03_child -n 400");
> +	if ((pid = SAFE_FORK()) == 0) {
> +		retval = tst_system("getrusage03_child consume 400");
>  		if ((WIFEXITED(retval) && WEXITSTATUS(retval) != 0))
> -			tst_brkm(TBROK | TERRNO, cleanup, "system");
> +			tst_brk(TBROK, "system(\"getrusage03_child consume 400\")");
>  		exit(0);
> -	default:
> -		break;
>  	}

Here as well, just use execve().

> -	sleep(1);		/* children become zombie */
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "pre_wait.children = %ld", ru.ru_maxrss);
> -	if (is_in_delta(ru.ru_maxrss - maxrss_init))
> -		tst_resm(TPASS, "initial.children ~= pre_wait.children");
> -	else
> -		tst_resm(TFAIL, "initial.children !~= pre_wait.children");
> -
> -	SAFE_WAITPID(cleanup, pid, &status, WUNTRACED | WCONTINUED);
> -	if (WEXITSTATUS(status) != 0)
> -		tst_brkm(TBROK | TERRNO, cleanup, "child exit status is not 0");
> +	TST_PROCESS_STATE_WAIT(pid, 'Z', 0);
> +	SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +	check_result(is_in_delta(ru.ru_maxrss - maxrss_init),
> +		     "check that initial.children ~= pre_wait.children");

Here as well, just print TPASS/TFAIL messages rather than using the
check_result() function.

> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "post_wait.children = %ld", ru.ru_maxrss);
> -	if (is_in_delta(ru.ru_maxrss - 409600))
> -		tst_resm(TPASS, "post_wait.children ~= 400MB");
> -	else
> -		tst_resm(TFAIL, "post_wait.children !~= 400MB");
> +	tst_reap_children();
> +	SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +	check_result(is_in_delta(ru.ru_maxrss - 409600), "check that post_wait.children ~= 400MB");

And here as well.

>  }
> 
> -/* Testcase #06: SIG_IGN
> - * expect: initial ~= after_zombie */
>  static void sig_ign(void)
>  {
> -	tst_resm(TINFO, "Testcase #06: SIG_IGN");
> -
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "initial.children = %ld", ru.ru_maxrss);
> -	signal(SIGCHLD, SIG_IGN);
> +	SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
>  	maxrss_init = ru.ru_maxrss;
> +	SAFE_SIGNAL(SIGCHLD, SIG_IGN);
> 
> -	switch (pid = fork()) {
> -	case -1:
> -		tst_brkm(TBROK, cleanup, "fork #6");
> -	case 0:
> -		retval = system("getrusage03_child -n 500");
> +	if ((pid = SAFE_FORK()) == 0) {
> +		retval = tst_system("getrusage03_child consume 500");
>  		if ((WIFEXITED(retval) && WEXITSTATUS(retval) != 0))
> -			tst_brkm(TBROK | TERRNO, cleanup, "system");
> +			tst_brk(TBROK, "system(\"getrusage03_child consume 500\")");
>  		exit(0);
> -	default:
> -		break;
>  	}


And here as well.

> -	sleep(1);		/* children become zombie */
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	tst_resm(TINFO, "after_zombie.children = %ld", ru.ru_maxrss);
> -	if (is_in_delta(ru.ru_maxrss - maxrss_init))
> -		tst_resm(TPASS, "initial.children ~= after_zombie.children");
> -	else
> -		tst_resm(TFAIL, "initial.children !~= after_zombie.children");
> -	signal(SIGCHLD, SIG_DFL);
> +	TST_PROCESS_RELEASE_WAIT(pid, 0);

Why can't we use the TST_PROCESS_STATE_WAIT(pid, 'Z', 0) here as well? I
fail to see how this is different from the previous test.

> +	SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +	check_result(is_in_delta(ru.ru_maxrss - maxrss_init),
> +		     "check that initial.children ~= after_zombie.children");
> +	SAFE_SIGNAL(SIGCHLD, SIG_DFL);
>  }
> 
> -/* Testcase #07: exec without fork
> - * expect: initial ~= fork */
> -static void exec_without_fork(void)
> +static void inherit_exec(void)
>  {
> -	char str_maxrss_self[BUFSIZ], str_maxrss_child[BUFSIZ];
> -	long maxrss_self, maxrss_child;
> +	int status;
> 
> -	tst_resm(TINFO, "Testcase #07: exec without fork");
> +	if ((pid = SAFE_FORK()) == 0) {
> +		char str_maxrss_self[BUFSIZ], str_maxrss_child[BUFSIZ];
> 
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_SELF, &ru);
> -	maxrss_self = ru.ru_maxrss;
> -	SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -	maxrss_child = ru.ru_maxrss;
> -	tst_resm(TINFO, "initial.self = %ld, initial.children = %ld",
> -		 maxrss_self, maxrss_child);
> +		SAFE_GETRUSAGE(RUSAGE_SELF, &ru);
> +		sprintf(str_maxrss_self, "%ld", ru.ru_maxrss);
> +		SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +		sprintf(str_maxrss_child, "%ld", ru.ru_maxrss);
> 
> -	sprintf(str_maxrss_self, "%ld", maxrss_self);
> -	sprintf(str_maxrss_child, "%ld", maxrss_child);
> -	if (execlp("getrusage03_child", "getrusage03_child", "-v",
> -		   "-s", str_maxrss_self, "-l", str_maxrss_child, NULL) == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup, "execlp");
> -}
> +		SAFE_EXECLP("getrusage03_child", "getrusage03_child", "compare",
> +			    str_maxrss_self, str_maxrss_child, NULL);
> +	}
> 
> -static int is_in_delta(long value)
> -{
> -	return (value >= -DELTA_MAX && value <= DELTA_MAX);
> +	SAFE_WAITPID(pid, &status, 0);
> +	check_result(WIFEXITED(status) && WEXITSTATUS(status) == 0,
> +		     "check that initial.self ~= exec.self, initial.children ~= exec.children");

In this case the check is done in the child. All the parent has to do is
to reap the children to make sure that it did exit with 0 and the child
should report the result instead.

As the child calls tst_reinit() it can report results directly to the
main test process without need to propagate anything via return values.

>  }
> 
> -static void check_return(int status, char *pass_msg, char *fail_msg)
> +static void run(void)
>  {
> -	switch (status) {
> -	case 1:
> -		tst_resm(TPASS, "%s", pass_msg);
> -		break;
> -	case 0:
> -		tst_resm(TFAIL, "%s", fail_msg);
> -		break;
> -	default:
> -		tst_resm(TFAIL, "child exit status is %d", status);
> -		break;
> -	}
> -}
> -
> -static void consume(int mega)
> -{
> -	size_t sz;
> -	void *ptr;
> -
> -	sz = mega * 1024 * 1024;
> -	ptr = SAFE_MALLOC(cleanup, sz);
> -	memset(ptr, 0, sz);
> +	inherit_fork1();
> +	inherit_fork2();
> +	grandchild_maxrss();
> +	zombie();
> +	sig_ign();
> +	inherit_exec();

Can we split these into a several tests?

Have a look at snd_seq01.c and testfunc_list array how this is done.

>  }
> 
>  static void setup(void)
>  {
> -	/* Disable test if the version of the kernel is less than 2.6.32 */
> -	if ((tst_kvercmp(2, 6, 32)) < 0) {
> -		tst_resm(TCONF, "This ru_maxrss field is not supported");
> -		tst_brkm(TCONF, NULL, "before kernel 2.6.32");
> -	}
> -
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> +	consume(100);
>  }
> 
> -static void cleanup(void)
> -{
> -}
> +static struct tst_test test = {
> +	.forks_child = 1,
> +	.child_needs_reinit = 1,
> +	.resource_files = resource,
> +	.min_kver = "2.6.32",
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "1f10206cf8e9"},
> +	},
> +	.setup = setup,
> +	.test_all = run,
> +};
> diff --git a/testcases/kernel/syscalls/getrusage/getrusage03.h b/testcases/kernel/syscalls/getrusage/getrusage03.h
> new file mode 100644
> index 000000000..5fbf57272
> --- /dev/null
> +++ b/testcases/kernel/syscalls/getrusage/getrusage03.h
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2011  Red Hat, Inc.
> + * Copyright (c) 2021 Xie Ziyao <xieziyao@huawei.com>
> + */
> +
> +#ifndef LTP_GETRUSAGE03_H
> +#define LTP_GETRUSAGE03_H
> +
> +#include "tst_test.h"
> +
> +#define DELTA_MAX 20480
> +
> +static void consume(int consume_nr)
               ^
	       This should be called consume_mb() so that it's clear
	       that the parameter is in megabytes.
> +{
> +	void* ptr;
> +	size_t size;
> +	unsigned long vmswap_size;
> +
> +	size = consume_nr * 1024 * 1024;
> +	ptr = SAFE_MALLOC(size);
> +	memset(ptr, 0, size);
> +
> +	SAFE_FILE_LINES_SCANF("/proc/self/status", "VmSwap: %lu", &vmswap_size);
> +	if (vmswap_size > 0)
> +		tst_brk(TBROK, "VmSwap is not zero");
> +	tst_res(TINFO, "allocate %dMB", consume_nr);
> +}
> +
> +static int is_in_delta(long value)
> +{
> +	return (value >= -DELTA_MAX && value <= DELTA_MAX);
> +}
> +
> +#endif //LTP_GETRUSAGE03_H
> diff --git a/testcases/kernel/syscalls/getrusage/getrusage03_child.c b/testcases/kernel/syscalls/getrusage/getrusage03_child.c
> index 972c38e4e..58da481cb 100644
> --- a/testcases/kernel/syscalls/getrusage/getrusage03_child.c
> +++ b/testcases/kernel/syscalls/getrusage/getrusage03_child.c
> @@ -1,168 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - * getrusage03_child.c - a child program executed by getrusage03
> - *
>   * Copyright (C) 2011  Red Hat, Inc.
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of version 2 of the GNU General Public
> - * License as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> - *
> - * Further, this software is distributed without any warranty that it
> - * is free of the rightful claim of any third person regarding
> - * infringement or the like.  Any license provided herein, whether
> - * implied or otherwise, applies only to this software file.  Patent
> - * licenses, if any, provided herein do not apply to combinations of
> - * this program with other software, or any other product whatsoever.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> - * 02110-1301, USA.
> + * Copyright (c) 2021 Xie Ziyao <xieziyao@huawei.com>
>   */
> -#include <sys/types.h>
> -#include <sys/resource.h>
> -#include <sys/time.h>
> -#include <sys/wait.h>
> -#include <errno.h>
> -#include <unistd.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <limits.h>
> -
> -#include "test.h"
> -#include "safe_macros.h"
> 
> -char *TCID = "getrusage03_child";
> -int TST_TOTAL = 1;
> -
> -#define DELTA_MAX	10240
> +/*\
> + * [Description]
> + *
> + * Child program executed by getrusage03.
> + */
> 
> -static int opt_consume, opt_grand, opt_show, opt_self, opt_child;
> -static char *consume_str, *grand_consume_str, *self_str, *child_str;
> +#define TST_NO_DEFAULT_MAIN
> 
> -option_t child_options[] = {
> -	{"n:", &opt_consume, &consume_str},
> -	{"g:", &opt_grand, &grand_consume_str},
> -	{"v", &opt_show, NULL},
> -	{"s:", &opt_self, &self_str},
> -	{"l:", &opt_child, &child_str},
> -	{NULL, NULL, NULL}
> -};
> +#include <stdlib.h>
> 
> -static void usage(void);
> -static void consume(int mega);
> -static void setup(void);
> -static void cleanup(void);
> +#include "tst_test.h"
> +#include "getrusage03.h"
> 
>  int main(int argc, char *argv[])
>  {
> -	int lc;
> +	if (argc < 3)
> +		tst_brk(TFAIL, "argc is %d, expected more", argc);

This is TBROK and also the message could be more clear such as:

	"expected at least two parameters"

>  	pid_t pid;
> -	long maxrss_self, maxrss_children, delta;
> +	long maxrss_self, maxrss_children;
>  	long consume_nr, grand_consume_nr, self_nr, child_nr;
>  	struct rusage ru;
> 
> -	tst_parse_opts(argc, argv, child_options, usage);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -
> -		if (opt_consume) {
> -			consume_nr = SAFE_STRTOL(cleanup,
> -						 consume_str, 0, LONG_MAX);
> -			tst_resm(TINFO, "child allocate %ldMB", consume_nr);
> -			consume(consume_nr);
> -		}
> +	tst_reinit();
> 
> -		if (opt_grand) {
> -			grand_consume_nr = SAFE_STRTOL(cleanup,
> -						       grand_consume_str, 0,
> -						       LONG_MAX);
> -			tst_resm(TINFO, "grandchild allocate %ldMB",
> -				 grand_consume_nr);
> -			switch (pid = fork()) {
> -			case -1:
> -				tst_brkm(TBROK, cleanup, "fork");
> -			case 0:
> -				consume(grand_consume_nr);
> -				exit(0);
> -			default:
> -				break;
> -			}
> -			while (waitpid(-1, &pid, WUNTRACED | WCONTINUED) > 0)
> -				if (WEXITSTATUS(pid) != 0)
> -					tst_brkm(TBROK | TERRNO, cleanup,
> -						 "child exit status is not 0");
> +	if (!strcmp(argv[1], "consume")) {
> +		consume_nr = SAFE_STRTOL(argv[2], 0, LONG_MAX);
> +		consume(consume_nr);
> +	}
> +	else if (!strcmp(argv[1], "grand_consume")) {

The else has to be on the same line as }

> +		grand_consume_nr = SAFE_STRTOL(argv[2], 0, LONG_MAX);
> +
> +		pid = fork();
> +		if (pid == -1)
> +			tst_brk(TBROK, "fork failed");
> +		else if (pid == 0) {
> +			consume(grand_consume_nr);
> +			exit(0);
>  		}

Just use SAFE_FORK() here.

> -		if (opt_show) {
> -			SAFE_GETRUSAGE(cleanup, RUSAGE_SELF, &ru);
> -			maxrss_self = ru.ru_maxrss;
> -			SAFE_GETRUSAGE(cleanup, RUSAGE_CHILDREN, &ru);
> -			maxrss_children = ru.ru_maxrss;
> -			tst_resm(TINFO, "exec.self = %ld, exec.children = %ld",
> -				 maxrss_self, maxrss_children);
> -			if (opt_self) {
> -				self_nr = SAFE_STRTOL(cleanup,
> -						      self_str, 0, LONG_MAX);
> -				delta = maxrss_self - self_nr;
> -				if (delta >= -DELTA_MAX && delta <= DELTA_MAX)
> -					tst_resm(TPASS,
> -						 "initial.self ~= exec.self");
> -				else
> -					tst_resm(TFAIL,
> -						 "initial.self !~= exec.self");
> -			}
> -			if (opt_child) {
> -				child_nr = SAFE_STRTOL(cleanup,
> -						       child_str, 0, LONG_MAX);
> -				delta = maxrss_children - child_nr;
> -				if (delta >= -DELTA_MAX && delta <= DELTA_MAX)
> -					tst_resm(TPASS,
> -						 "initial.children ~= exec.children");
> -				else
> -					tst_resm(TFAIL,
> -						 "initial.children !~= exec.children");
> -			}
> -		}
> +		tst_reap_children();
>  	}
> +	else if (!strcmp(argv[1], "compare")) {
> +		self_nr = SAFE_STRTOL(argv[2], 0, LONG_MAX);
> +		child_nr = SAFE_STRTOL(argv[3], 0, LONG_MAX);
> 
> -	cleanup();
> -	tst_exit();
> -}
> -
> -static void usage(void)
> -{
> -	printf("  -n NUM  consume NUM MB size\n");
> -	printf("  -g NUM  grandchild consume NUM MB size\n");
> -	printf("  -v      verbose mode, show rusage info\n");
> -	printf("  -s NUM  compare rusage_self.maxrss with given NUM\n");
> -	printf("  -l NUM  compare rusage_children.maxrss with given NUM\n");
> -}
> -
> -static void consume(int mega)
> -{
> -	size_t sz;
> -	void *ptr;
> +		SAFE_GETRUSAGE(RUSAGE_SELF, &ru);
> +		maxrss_self = ru.ru_maxrss;
> +		SAFE_GETRUSAGE(RUSAGE_CHILDREN, &ru);
> +		maxrss_children = ru.ru_maxrss;
> 
> -	sz = mega * 1024 * 1024;
> -	ptr = SAFE_MALLOC(cleanup, sz);
> -	memset(ptr, 0, sz);
> -}
> -
> -static void setup(void)
> -{
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> -}
> +		if (!is_in_delta(maxrss_self - self_nr) || !is_in_delta(maxrss_children - child_nr))
> +			tst_brk(TBROK, "check that initial.self ~= exec.self, initial.children ~= exec.children");

This should produce TPASS/TFAIL messages and for each of them
respectivelly as:

		if (is_in_delta(maxrss_self - self_nr))
			tst_res(TPASS, ...);
		else
			tst_res(TFAIL, ...);

		if (is_in_delta(maxrss_children - child_nr))
			tst_res(TPASS, ...);
		else
			tst_res(TFAIL, ...);
	}

	return 0;

> +	}
> 
> -static void cleanup(void)
> -{
> +	return 0;
>  }
> --
> 2.17.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

  reply	other threads:[~2021-06-16 11:47 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-16  9:36 [LTP] [PATCH 0/3] Cleanup and bugfix for getrusage{01, 03} Xie Ziyao
2021-06-16  9:36 ` [LTP] [PATCH 1/3] lib: tst_process_state: Add tst_process_release_wait() Xie Ziyao
2021-06-16 11:04   ` Cyril Hrubis
2021-06-16 11:54     ` Xie Ziyao
2021-06-16  9:36 ` [LTP] [PATCH 2/3] syscalls/getrusage: Cleanup and bugfix for getrusage03 Xie Ziyao
2021-06-16 11:47   ` Cyril Hrubis [this message]
2021-06-16 13:13     ` Xie Ziyao
2021-06-16 13:13       ` Cyril Hrubis
2021-06-16  9:36 ` [LTP] [PATCH 3/3] syscalls/getrusage: Convert getrusage01 to the new API Xie Ziyao
2021-06-16 11:56   ` Cyril Hrubis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YMnk5QUhwnOHrWLi@yuki \
    --to=chrubis@suse.cz \
    --cc=ltp@lists.linux.it \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.