Re: [LTP] [PATCH v2] Remove libclone dependency from pidns05 test

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

> From: Andrea Cervesato <andrea.cervesato@suse.com>

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
nit: with so many inputs you get from Cyril, it'd really help reviewers if you
wrote changelog (nice example: https://lore.kernel.org/ltp/20230711170335.13142-1-rick.p.edgecombe@intel.com/).

I agree it's tedious and boring but not only it'd help reviewers, but it'd help
you to check if you changed everything you had been asked to.

>  testcases/kernel/containers/pidns/pidns05.c | 285 ++++++--------------
>  1 file changed, 77 insertions(+), 208 deletions(-)

> diff --git a/testcases/kernel/containers/pidns/pidns05.c b/testcases/kernel/containers/pidns/pidns05.c
> index 79e146e36..d3be4be11 100644
> --- a/testcases/kernel/containers/pidns/pidns05.c
> +++ b/testcases/kernel/containers/pidns/pidns05.c
> @@ -1,256 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> -* Copyright (c) International Business Machines Corp., 2007
> -* This program is free software; you can redistribute it and/or modify
> -* it under the terms of the GNU General Public License as published by
> -* the Free Software Foundation; either version 2 of the License, or
> -* (at your option) any later version.
> -* This program is distributed in the hope that it will be useful
> -* but WITHOUT ANY WARRANTY; without even the implied warranty of
> -* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> -* the GNU General Public License for more details.
> -* You should have received a copy of the GNU General Public License
> -* along with this program; if not, write to the Free Software
> -* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> -*
> -***************************************************************************
> -*
> -* Assertion:
> -*   a) Create a  container.
> -*   b) Create many levels of child containers inside this container.
> -*   c) Now do kill -9 init , outside of the container.
> -*   d) This should kill all the child containers.
> -*      (containers created at the level below)
> -*
> -* Description:
> -* 1. Parent process clone a process with flag CLONE_NEWPID
> -* 2. The container will recursively loop and creates 4 more containers.
> -* 3. All the container init's  goes into sleep(), waiting to be terminated.
> -* 4. The parent process will kill child[3] by passing SIGKILL
> -* 5. Now parent process, verifies the child containers 4 & 5 are destroyed.
> -* 6. If they are killed then
> -*	Test passed
> -*  else Test failed.
> -*
> -* Test Name: pidns05
> -*
> -* History:
> -*
> -* FLAG DATE		NAME				DESCRIPTION
> -* 31/10/08  Veerendra C <vechandr@in.ibm.com>	Verifies killing of NestedCont's
> -*
> -*******************************************************************************/
> -#define _GNU_SOURCE 1
> + * Copyright (c) International Business Machines Corp., 2007
> + *		08/10/08 Veerendra C <vechandr@in.ibm.com>
very nit: 31/10/08 => 08/10/08. IMHO wrong date. Or what am I missing here?
Also why tab in the front? Maybe I'm a bit aggressive when deleting from my POV
useless info and ugly formatting, but I'd just add:

* Copyright (c) Veerendra C <vechandr@in.ibm.com>, 2008

> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
...

+static int find_cinit_pids(pid_t *pids)
+{
+	int i;
+    int next = 0;
+    pid_t parentpid, pgid, pgid2;
+
+    parentpid = getpid();
+    pgid = getpgid(parentpid);
Maybe SAFE_GETPGID() ?

...

> +static void run(void)
...
> +	SAFE_KILL(pid, SIGKILL);
> +	SAFE_WAITPID(0, &status, NULL);

In file included from ../../../../include/tst_test.h:110,
                 from pidns05.c:17:
pidns05.c: In function ‘run’:
../../../../include/tst_safe_macros.h:211:65: warning: passing argument 6 of ‘safe_waitpid’ makes integer from pointer without a cast [-Wint-conversion]
  211 |         safe_waitpid(__FILE__, __LINE__, NULL, (pid), (status), (opts))
      |                                                                 ^~~~~~
      |                                                                 |
      |                                                                 void *
pidns05.c:101:9: note: in expansion of macro ‘SAFE_WAITPID’
  101 |         SAFE_WAITPID(0, &status, NULL);
      |         ^~~~~~~~~~~~
In file included from ../../../../include/tst_safe_macros.h:24:
../../../../include/safe_macros_fn.h:158:48: note: expected ‘int’ but argument is of type ‘void *’
  158 |                    pid_t pid, int *status, int opts);
      |                                            ~~~~^~~~

make check-pidns05
CHECK testcases/kernel/containers/pidns/pidns05.c
pidns05.c:101:9: warning: incorrect type in argument 6 (different base types)
pidns05.c:101:9:    expected int opts
pidns05.c:101:9:    got void *

The old test called setpgid(0, 0) and SAFE_PIPE(), are these related to
do_clone_unshare_test() or why it's not necessary in the rewrite?
Maybe it's obvious, but I'd appreciate to explain these changes in the commit
message, instead of simple "Remove libclone dependency".

The rest LGTM.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp