Re: [LTP] [PATCH v6 1/6] Refactor mqns_01 using new LTP API

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi!

On 3/23/23 11:32, Petr Vorel wrote:
> Hi Andrea,
>
>> diff --git a/runtest/containers b/runtest/containers
>> index 23f394579..5e1b53bfa 100644
>> --- a/runtest/containers
>> +++ b/runtest/containers
>> @@ -16,7 +16,8 @@ pidns31 pidns31
>>   pidns32 pidns32
>>   mqns_01 mqns_01
>> -mqns_01_clone mqns_01 -clone
>> +mqns_01_clone mqns_01 -m clone
> I'd be for renaming these testcases to rename the underscore (e.g. from
> mqns_01.c to mqns01.c). But this can be done any time later.
There are so many tests without convention. We should create it before 
changing this test specifically.
>
>> +mqns_01_unshare mqns_01 -m unshare
>>   mqns_02 mqns_02
>>   mqns_02_clone mqns_02 -clone
>>   mqns_03 mqns_03
>> diff --git a/testcases/kernel/containers/mqns/mqns_01.c b/testcases/kernel/containers/mqns/mqns_01.c
>> index 1d109e020..61e586c14 100644
>> --- a/testcases/kernel/containers/mqns/mqns_01.c
>> +++ b/testcases/kernel/containers/mqns/mqns_01.c
>> @@ -1,148 +1,82 @@
>> +// SPDX-License-Identifier: GPL-2.0
> It should be GPL-2.0-or-later
>
> I'm going to fix it before merge.
>
>>   /*
>> -* Copyright (c) International Business Machines Corp., 2009
>> -* Copyright (c) Nadia Derbey, 2009
>> -* 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.
>> -*
>> -* Check mqns isolation: father mqns cannot be accessed from newinstance
>> -*
>> -* Mount mqueue fs
>> -* Create a posix mq -->mq1
>> -* unshare
>> -* In unshared process:
>> -*    Mount newinstance mqueuefs
>> -*    Check that mq1 is not readable from new ns
>> + * Copyright (c) International Business Machines Corp., 2009
>> + * Copyright (c) Nadia Derbey, 2009 <Nadia.Derbey@bull.net>
>> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
>> + */
>> -***************************************************************************/
>> +/*\
>> + * [Description]
>> + *
>> + * Create a mqueue inside the parent and check if it can be accessed from
> maybe:
> Create a mqueue inside the parent and verify it cannot be accessed from
> the child namespace.
>
>> + * the child namespace.
>> + */
>> -#ifndef _GNU_SOURCE
>> -#define _GNU_SOURCE
>> -#endif
>> -#include <sys/wait.h>
>> -#include <errno.h>
>> -#include <stdio.h>
>> -#include <stdlib.h>
>> -#include <string.h>
>> -#include <unistd.h>
>> -#include "mqns.h"
>> -#include "mqns_helper.h"
>> +#include "tst_test.h"
>> +#include "lapi/sched.h"
>> +#include "tst_safe_posix_ipc.h"
>> -char *TCID = "posixmq_namespace_01";
>> -int TST_TOTAL = 1;
>> +#define MQNAME "/MQ1"
>> -int p1[2];
>> -int p2[2];
>> +static mqd_t mqd;
>> +static char *str_op;
>> -int check_mqueue(void *vtest)
>> +static void run(void)
>>   {
>> -	char buf[30];
>> -	mqd_t mqd;
>> +	const struct tst_clone_args clone_args = { CLONE_NEWIPC, SIGCHLD };
>> -	(void) vtest;
>> +	tst_res(TINFO, "Checking namespaces isolation from parent to child");
>> -	close(p1[1]);
>> -	close(p2[0]);
>> +	if (str_op && !strcmp(str_op, "clone")) {
>> +		tst_res(TINFO, "Spawning isolated process");
>> -	if (read(p1[0], buf, strlen("go") + 1) < 0) {
>> -		printf("read(p1[0], ...) failed: %s\n", strerror(errno));
>> -		exit(1);
>> -	}
>> -	mqd = tst_syscall(__NR_mq_open, NOSLASH_MQ1, O_RDONLY);
>> -	if (mqd == -1) {
>> -		if (write(p2[1], "notfnd", strlen("notfnd") + 1) < 0) {
>> -			perror("write(p2[1], ...) failed");
>> -			exit(1);
>> +		if (!SAFE_CLONE(&clone_args)) {
>> +			TST_EXP_FAIL(mq_open(MQNAME, O_RDONLY), ENOENT);
>> +			return;
>> +		}
>> +	} else if (str_op && !strcmp(str_op, "unshare")) {
>> +		tst_res(TINFO, "Spawning unshared process");
>> +
>> +		if (!SAFE_FORK()) {
>> +			SAFE_UNSHARE(CLONE_NEWIPC);
>> +			TST_EXP_FAIL(mq_open(MQNAME, O_RDONLY), ENOENT);
>> +			return;
>>   		}
>>   	} else {
>> -		if (write(p2[1], "exists", strlen("exists") + 1) < 0) {
>> -			perror("write(p2[1], \"exists\", 7) failed");
>> -			exit(1);
>> -		} else if (mq_close(mqd) < 0) {
>> -			perror("mq_close(mqd) failed");
>> -			exit(1);
>> +		tst_res(TINFO, "Spawning plain process");
>> +
>> +		if (!SAFE_FORK()) {
>> +			TST_EXP_POSITIVE(mq_open(MQNAME, O_RDONLY));
>> +			return;
>>   		}
>>   	}
>> -
>> -	exit(0);
>>   }
>>   static void setup(void)
>>   {
>> -	tst_require_root();
>> -	check_mqns();
>> +	mqd = SAFE_MQ_OPEN(MQNAME, O_RDWR | O_CREAT | O_EXCL, 0777, NULL);
>>   }
>> -int main(int argc, char *argv[])
>> +static void cleanup(void)
>>   {
>> -	int r;
>> -	mqd_t mqd;
>> -	char buf[30];
>> -	int use_clone = T_UNSHARE;
>> -
>> -	setup();
>> -
>> -	if (argc == 2 && strcmp(argv[1], "-clone") == 0) {
>> -		tst_resm(TINFO,
>> -			 "Testing posix mq namespaces through clone(2).");
>> -		use_clone = T_CLONE;
>> -	} else
>> -		tst_resm(TINFO,
>> -			 "Testing posix mq namespaces through unshare(2).");
>> -
>> -	if (pipe(p1) == -1 || pipe(p2) == -1) {
>> -		tst_brkm(TBROK | TERRNO, NULL, "pipe failed");
>> -	}
>> -
>> -	mqd = tst_syscall(__NR_mq_open, NOSLASH_MQ1, O_RDWR | O_CREAT | O_EXCL,
>> -		0777, NULL);
>> -	if (mqd == -1) {
>> -		perror("mq_open");
>> -		tst_brkm(TFAIL, NULL, "mq_open failed");
>> +	if (mqd != -1) {
>> +		SAFE_MQ_CLOSE(mqd);
>> +		SAFE_MQ_UNLINK(MQNAME);
>>   	}
>> -
>> -	tst_resm(TINFO, "Checking namespaces isolation from parent to child");
>> -	/* fire off the test */
>> -	r = do_clone_unshare_test(use_clone, CLONE_NEWIPC, check_mqueue, NULL);
>> -	if (r < 0) {
>> -		tst_resm(TFAIL, "failed clone/unshare");
>> -		mq_close(mqd);
>> -		tst_syscall(__NR_mq_unlink, NOSLASH_MQ1);
>> -		tst_exit();
>> -	}
>> -
>> -	close(p1[0]);
>> -	close(p2[1]);
>> -	if (write(p1[1], "go", strlen("go") + 1) < 0)
>> -		tst_resm(TBROK | TERRNO, "write(p1[1], \"go\", ...) failed");
> The old version actually communicate between parent and child proces over pipe
> (p1, p2 arrays). Is it all of these useless? I wonder myself what have POSIX
> message queues together with pipes.
pipes were used because LTP didn't support tests result from children.
> I'm sorry if I overlook discussion about it in the past (looked at various
> versions; it'd be worth to mention changes like this in the commit message).
>
> Kind regards,
> Petr
>
>> -	else if (read(p2[0], buf, 7) < 0)
>> -		tst_resm(TBROK | TERRNO, "read(p2[0], buf, ...) failed");
>> -	else {
>> -		if (!strcmp(buf, "exists")) {
>> -			tst_resm(TFAIL, "child process found mqueue");
>> -		} else if (!strcmp(buf, "notfnd")) {
>> -			tst_resm(TPASS, "child process didn't find mqueue");
>> -		} else {
>> -			tst_resm(TFAIL, "UNKNOWN RESULT");
>> -		}
>> -	}
>> -
>> -	/* destroy the mqueue */
>> -	if (mq_close(mqd) == -1) {
>> -		tst_brkm(TBROK | TERRNO, NULL, "mq_close failed");
>> -	}
>> -	tst_syscall(__NR_mq_unlink, NOSLASH_MQ1);
>> -
>> -	tst_exit();
>>   }
>> +
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.needs_root = 1,
>> +	.forks_child = 1,
>> +	.options = (struct tst_option[]) {
>> +		{ "m:", &str_op, "Child process isolation <clone|unshare>" },
>> +		{},
>> +	},
>> +	.needs_kconfigs = (const char *[]) {
>> +		"CONFIG_USER_NS",
>> +		NULL
>> +	},
>> +};

Andrea


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