[LTP] [PATCH v1] Refactor pidns05 test using new LTP API
Andrea Cervesato
andrea.cervesato@suse.com
Tue Feb 7 12:46:51 CET 2023
Hi,
On 2/7/23 11:51, Richard Palethorpe wrote:
> Hello,
>
> Andrea Cervesato <andrea.cervesato@suse.com> writes:
>
>> Hi!
>>
>> Can we merge this patch anyway? ltp_clone should be added after with a
>> single patch.
> I started work on an update to tst_clone so that it can replace most
> intances of ltp_clone. However it appears that it can already replace
> most instances of ltp_clone_quick.
>
> AFAICT all you need to do is get rid of child_func and use tst_clone
> like fork.
>
> I don't remember if there is anything else in this patch that needs
> reviewing, so there is no guarantee that when I look at this again it
> will get merged.
>
> So I don't see any advantage to deferring the change to tst_clone.
I sent a v2, please take a look at that one. I can start using tst_clone
for the next reviews if needed.
>> Andrea
>>
>> On 10/11/22 11:56, Richard Palethorpe wrote:
>>> Hello,
>>>
>>> Andrea Cervesato via ltp <ltp@lists.linux.it> writes:
>>>
>>>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>>>> ---
>>>> testcases/kernel/containers/pidns/pidns05.c | 288 ++++++--------------
>>>> 1 file changed, 78 insertions(+), 210 deletions(-)
>>>>
>>>> diff --git a/testcases/kernel/containers/pidns/pidns05.c b/testcases/kernel/containers/pidns/pidns05.c
>>>> index 79e146e36..1c588991b 100644
>>>> --- a/testcases/kernel/containers/pidns/pidns05.c
>>>> +++ b/testcases/kernel/containers/pidns/pidns05.c
>>>> @@ -1,256 +1,124 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> /*
>>>> -* 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>
>>>> + * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
>>>> + */
>>>> +
>>>> +/*\
>>>> + * [Description]
>>>> + *
>>>> + * Clone a process with CLONE_NEWPID flag and create many levels of child
>>>> + * containers. Then kill container init process from parent and check if all
>>>> + * containers have been killed.
>>>> + */
>>>> +
>>>> #include <sys/wait.h>
>>>> -#include <assert.h>
>>>> -#include <stdio.h>
>>>> -#include <stdlib.h>
>>>> -#include <unistd.h>
>>>> -#include <string.h>
>>>> -#include <errno.h>
>>>> -#include "pidns_helper.h"
>>>> -#include "test.h"
>>>> -#include "safe_macros.h"
>>>> +#include "tst_test.h"
>>>> +#include "lapi/namespaces_constants.h"
>>>> -#define INIT_PID 1
>>>> -#define CINIT_PID 1
>>>> -#define PARENT_PID 0
>>>> #define MAX_DEPTH 5
>>>> -char *TCID = "pidns05";
>>>> -int TST_TOTAL = 1;
>>>> -int fd[2];
>>>> +static pid_t pid_max;
>>>> -int max_pid(void)
>>>> +static int child_func(void *arg)
>>>> {
>>>> - FILE *fp;
>>>> int ret;
>>>> + int *level;
>>>> + pid_t cpid, ppid;
>>>> +
>>>> + cpid = getpid();
>>>> + ppid = getppid();
>>>> - fp = fopen("/proc/sys/kernel/pid_max", "r");
>>>> - if (fp != NULL) {
>>>> - fscanf(fp, "%d", &ret);
>>>> - fclose(fp);
>>>> - } else {
>>>> - tst_resm(TBROK, "Cannot open /proc/sys/kernel/pid_max");
>>>> - ret = -1;
>>>> + if (cpid != 1 || ppid != 0) {
>>>> + tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
>>>> + return 1;
>>>> }
>>>> - return ret;
>>>> +
>>>> + level = (int *)arg;
>>>> +
>>>> + if (*level >= MAX_DEPTH) {
>>>> + TST_CHECKPOINT_WAKE(0);
>>>> + return 0;
>>>> + }
>>>> +
>>>> + (*level)++;
>>>> +
>>>> + ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func,
>>>> level);
>>> Again, ltp_clone should be converted to tst_clone.
>>>
>>>> + if (ret < 0)
>>>> + tst_brk(TBROK | TERRNO, "clone failed");
>>>> +
>>>> + pause();
>>>> +
>>>> + return 0;
>>>> }
>>>> -/* find_cinit_pids() iteratively finds the pid's having same
>>>> PGID as its parent.
>>>> - * Input parameter - Accepts pointer to pid_t : To copy the pid's matching.
>>>> - * Returns - the number of pids matched.
>>>> -*/
>>>> -int find_cinit_pids(pid_t * pids)
>>>> +static int find_cinit_pids(pid_t *pids)
>>>> {
>>>> - int next = 0, pid_max, i;
>>>> + int i;
>>>> + int next = 0;
>>>> pid_t parentpid, pgid, pgid2;
>>>> - pid_max = max_pid();
>>>> parentpid = getpid();
>>>> pgid = getpgid(parentpid);
>>>> - /* The loop breaks, when the loop counter reaches the
>>>> parentpid value */
>>>> - for (i = parentpid + 1; i != parentpid; i++) {
>>>> - if (i > pid_max)
>>>> - i = 2;
>>>> -
>>>> + for (i = parentpid + 1; i < pid_max; i++) {
>>>> pgid2 = getpgid(i);
>>>> +
>>>> if (pgid2 == pgid) {
>>>> pids[next] = i;
>>>> next++;
>>>> }
>>>> }
>>>> +
>>>> return next;
>>>> }
>>>> -/*
>>>> -* create_nested_container() Recursively create MAX_DEPTH nested containers
>>>> -*/
>>>> -int create_nested_container(void *vtest)
>>>> +static void setup(void)
>>>> {
>>>> - int exit_val;
>>>> - int ret, count, *level;
>>>> - pid_t cpid, ppid;
>>>> - cpid = getpid();
>>>> - ppid = getppid();
>>>> - char mesg[] = "Nested Containers are created";
>>>> -
>>>> - level = (int *)vtest;
>>>> - count = *level;
>>>> -
>>>> - /* Child process closes up read side of pipe */
>>>> - close(fd[0]);
>>>> -
>>>> - /* Comparing the values to make sure pidns is created correctly */
>>>> - if (cpid != CINIT_PID || ppid != PARENT_PID) {
>>>> - printf("Got unexpected cpid and/or ppid (cpid=%d ppid=%d)\n",
>>>> - cpid, ppid);
>>>> - exit_val = 1;
>>>> - }
>>>> - if (count > 1) {
>>>> - count--;
>>>> - ret = do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>>>> - create_nested_container,
>>>> - (void *)&count);
>>>> - if (ret == -1) {
>>>> - printf("clone failed; errno = %d : %s\n",
>>>> - ret, strerror(ret));
>>>> - exit_val = 1;
>>>> - } else
>>>> - exit_val = 0;
>>>> - } else {
>>>> - /* Sending mesg, 'Nested containers created' through the pipe */
>>>> - write(fd[1], mesg, (strlen(mesg) + 1));
>>>> - exit_val = 0;
>>>> - }
>>>> -
>>>> - close(fd[1]);
>>>> - pause();
>>>> -
>>>> - return exit_val;
>>>> + SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%d\n", &pid_max);
>>>> }
>>>> -void kill_nested_containers()
>>>> +static void run(void)
>>>> {
>>>> - int orig_count, new_count, status = 0, i;
>>>> + int ret, i;
>>>> + int status;
>>>> + int children;
>>>> + int level = 0;
>>>> pid_t pids[MAX_DEPTH];
>>>> pid_t pids_new[MAX_DEPTH];
>>>> - orig_count = find_cinit_pids(pids);
>>>> - kill(pids[MAX_DEPTH - 3], SIGKILL);
>>>> - sleep(1);
>>>> -
>>>> - /* After killing child container, getting the New PID list */
>>>> - new_count = find_cinit_pids(pids_new);
>>>> + ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, &level);
>>>> + if (ret < 0)
>>>> + tst_brk(TBROK | TERRNO, "clone failed");
>>>> - /* Verifying that the child containers were destroyed when
>>>> parent is killed */
>>>> - if (orig_count - 2 != new_count)
>>>> - status = -1;
>>>> + TST_CHECKPOINT_WAIT(0);
>>>> - for (i = 0; i < new_count; i++) {
>>>> - if (pids[i] != pids_new[i])
>>>> - status = -1;
>>>> - }
>>>> + find_cinit_pids(pids);
>>>> - if (status == 0)
>>>> - tst_resm(TPASS, "The number of containers killed are %d",
>>>> - orig_count - new_count);
>>>> - else
>>>> - tst_resm(TFAIL, "Failed to kill the sub-containers of "
>>>> - "the container %d", pids[MAX_DEPTH - 3]);
>>>> -
>>>> - /* Loops through the containers created to exit from sleep() */
>>>> - for (i = 0; i < MAX_DEPTH; i++) {
>>>> - kill(pids[i], SIGKILL);
>>>> - waitpid(pids[i], &status, 0);
>>>> - }
>>>> -}
>>>> + SAFE_KILL(pids[0], SIGKILL);
>>>> -static void setup(void)
>>>> -{
>>>> - tst_require_root();
>>>> - check_newpid();
>>>> -}
>>>> + TST_RETRY_FUNC(waitpid(0, &status, WNOHANG), TST_RETVAL_NOTNULL);
>>>> -int main(void)
>>>> -{
>>>> - int ret, nbytes, status;
>>>> - char readbuffer[80];
>>>> - pid_t pid, pgid;
>>>> - int count = MAX_DEPTH;
>>>> + children = find_cinit_pids(pids_new);
>>>> - setup();
>>>> + if (children > 0) {
>>>> + tst_res(TFAIL, "%d children left after sending SIGKILL", children);
>>>> - /*
>>>> - * XXX (garrcoop): why in the hell is this fork-wait written this way?
>>>> - * This doesn't add up with the pattern used for the rest of the tests,
>>>> - * so I'm pretty damn sure this test is written incorrectly.
>>>> - */
>>>> - pid = fork();
>>>> - if (pid == -1) {
>>>> - tst_brkm(TBROK | TERRNO, NULL, "fork failed");
>>>> - } else if (pid != 0) {
>>>> - /*
>>>> - * NOTE: use waitpid so that we know we're waiting for the
>>>> - * _top-level_ child instead of a spawned subcontainer.
>>>> - *
>>>> - * XXX (garrcoop): Might want to mask SIGCHLD in the top-level
>>>> - * child too, or not *shrugs*.
>>>> - */
>>>> - if (waitpid(pid, &status, 0) == -1) {
>>>> - perror("wait failed");
>>>> + for (i = 0; i < MAX_DEPTH; i++) {
>>>> + kill(pids[i], SIGKILL);
>>>> + waitpid(pids[i], &status, 0);
>>>> }
>>>> - if (WIFEXITED(status))
>>>> - exit(WEXITSTATUS(status));
>>>> - else
>>>> - exit(status);
>>>> - }
>>>> - /* To make all the containers share the same PGID as its
>>>> parent */
>>>> - setpgid(0, 0);
>>>> -
>>>> - pid = getpid();
>>>> - pgid = getpgid(pid);
>>>> - SAFE_PIPE(NULL, fd);
>>>> -
>>>> - TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>>>> - create_nested_container, (void *)&count));
>>>> - if (TEST_RETURN == -1) {
>>>> - tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
>>>> + return;
>>>> }
>>>> - close(fd[1]);
>>>> - /* Waiting for the MAX_DEPTH number of containers to be created */
>>>> - nbytes = read(fd[0], readbuffer, sizeof(readbuffer));
>>>> - close(fd[0]);
>>>> - if (nbytes > 0)
>>>> - tst_resm(TINFO, " %d %s", MAX_DEPTH, readbuffer);
>>>> - else
>>>> - tst_brkm(TFAIL, NULL, "unable to create %d containers",
>>>> - MAX_DEPTH);
>>>> -
>>>> - /* Kill the container created */
>>>> - kill_nested_containers();
>>>> -
>>>> - tst_exit();
>>>> + tst_res(TPASS, "No children left after sending SIGKILL to the first child");
>>>> }
>>>> +
>>>> +static struct tst_test test = {
>>>> + .test_all = run,
>>>> + .setup = setup,
>>>> + .needs_root = 1,
>>>> + .needs_checkpoints = 1,
>>>> +};
>>>> -- 2.35.3
>
Andrea Cervesato
More information about the ltp
mailing list