Re: [LTP] [PATCH v7 3/6] Refactor mqns_03 using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +#include "tst_test.h"
> +#include "lapi/sched.h"
> +#include "tst_safe_posix_ipc.h"
> +#include "tst_safe_stdio.h"
> +
> +#define CHECK_MQ_OPEN_RET(x) ((x) >= 0 || ((x) == -1 && errno != EMFILE))
> +
> +#define MQNAME1 "/MQ1"
> +#define MQNAME2 "/MQ2"
> +
> +static char *str_op;
> +static char *devdir;
> +static char *mqueue1;
> +static char *mqueue2;
> +static int *mq_freed1;
> +static int *mq_freed2;
> +
> +static void check_mqueue(void)
>  {
> -	char buf[30];
> -	mqd_t mqd;
>  	int rc;
> +	mqd_t mqd;
>  	struct stat statbuf;
>  
> -	(void) vtest;
> +	tst_res(TINFO, "Creating %s mqueue from within child process", MQNAME1);
>  
> -	close(p1[1]);
> -	close(p2[0]);
> +	mqd = TST_RETRY_FUNC(
> +		mq_open(MQNAME1, O_RDWR | O_CREAT | O_EXCL, 0777, NULL),
> +		CHECK_MQ_OPEN_RET);
> +	if (mqd == -1)
> +		tst_brk(TBROK | TERRNO, "mq_open failed");
>  
> -	if (read(p1[0], buf, 3) != 3) {	/* go */
> -		perror("read failed");
> -		exit(1);
> -	}
> +	SAFE_MQ_CLOSE(mqd);
> +	tst_atomic_inc(mq_freed1);

I do not think that we need atomicity here, the cleanup code does not
run concurently at all as the cleanup in the parent is triggered after
the child did exit. I suppose that instead we need to set the mq_freed
to be volatile because it's shared memory which may change at any
change, so we need to tell that to the compiler.

> -	mqd = tst_syscall(__NR_mq_open, NOSLASH_MQ1, O_RDWR | O_CREAT | O_EXCL,
> -		0755, NULL);
> -	if (mqd == -1) {
> -		write(p2[1], "mqfail", 7);
> -		exit(1);
> -	}
> +	tst_res(TINFO, "Mount %s from within child process", devdir);
>  
> -	mq_close(mqd);
> +	SAFE_MOUNT("mqueue", devdir, "mqueue", 0, NULL);
>  
> -	rc = mount("mqueue", DEV_MQUEUE2, "mqueue", 0, NULL);
> -	if (rc == -1) {
> -		write(p2[1], "mount1", 7);
> -		exit(1);
> -	}
> +	SAFE_STAT(mqueue1, &statbuf);
> +	tst_res(TPASS, "%s exists at first mount", mqueue1);
>  
> -	rc = stat(FNAM1, &statbuf);
> -	if (rc == -1) {
> -		write(p2[1], "stat1", 6);
> -		exit(1);
> -	}
> +	tst_res(TINFO, "Creating %s from within child process", mqueue2);
>  
> -	rc = creat(FNAM2, 0755);
> -	if (rc == -1) {
> -		write(p2[1], "creat", 6);
> -		exit(1);
> -	}
> +	rc = SAFE_CREAT(mqueue2, 0755);
> +	SAFE_CLOSE(rc);
> +	tst_atomic_inc(mq_freed2);
>  
> -	close(rc);
> +	tst_res(TINFO, "Mount %s from within child process a second time", devdir);
>  
> -	rc = umount(DEV_MQUEUE2);
> -	if (rc == -1) {
> -		write(p2[1], "umount", 7);
> -		exit(1);
> -	}
> +	SAFE_UMOUNT(devdir);
> +	SAFE_MOUNT("mqueue", devdir, "mqueue", 0, NULL);
>  
> -	rc = mount("mqueue", DEV_MQUEUE2, "mqueue", 0, NULL);
> -	if (rc == -1) {
> -		write(p2[1], "mount2", 7);
> -		exit(1);
> -	}
> +	SAFE_STAT(mqueue1, &statbuf);
> +	tst_res(TPASS, "%s exists at second mount", mqueue1);
>  
> -	rc = stat(FNAM1, &statbuf);
> -	if (rc == -1) {
> -		write(p2[1], "stat2", 7);
> -		exit(1);
> -	}
> +	SAFE_STAT(mqueue2, &statbuf);
> +	tst_res(TPASS, "%s exists at second mount", mqueue2);
>  
> -	rc = stat(FNAM2, &statbuf);
> -	if (rc == -1) {
> -		write(p2[1], "stat3", 7);
> -		exit(1);
> -	}
> +	SAFE_UMOUNT(devdir);
> +
> +	SAFE_MQ_UNLINK(MQNAME1);
> +	tst_atomic_store(0, mq_freed1);
>  
> -	write(p2[1], "done", 5);
> +	SAFE_MQ_UNLINK(MQNAME2);
> +	tst_atomic_store(0, mq_freed2);
> +}
>  
> -	exit(0);
> +static void run(void)
> +{
> +	const struct tst_clone_args clone_args = { CLONE_NEWIPC, SIGCHLD };
> +
> +	if (str_op && !strcmp(str_op, "clone")) {
> +		tst_res(TINFO, "Spawning isolated process");
> +
> +		if (!SAFE_CLONE(&clone_args)) {
> +			check_mqueue();
> +			return;
> +		}
> +	} else if (str_op && !strcmp(str_op, "unshare")) {
> +		tst_res(TINFO, "Spawning unshared process");
> +
> +		if (!SAFE_FORK()) {
> +			SAFE_UNSHARE(CLONE_NEWIPC);
> +			check_mqueue();
> +			return;
> +		}
> +	}
>  }
>  
>  static void setup(void)
>  {
> -	tst_require_root();
> -	check_mqns();
> +	char *tmpdir;
> +
> +	if (!str_op)
> +		tst_brk(TCONF, "Please specify clone|unshare child isolation");
> +
> +	tmpdir = tst_get_tmpdir();
> +
> +	SAFE_ASPRINTF(&devdir, "%s/mqueue", tmpdir);
> +	SAFE_MKDIR(devdir, 0755);
> +
> +	SAFE_ASPRINTF(&mqueue1, "%s" MQNAME1, devdir);
> +	SAFE_ASPRINTF(&mqueue2, "%s" MQNAME2, devdir);
> +
> +	mq_freed1 = SAFE_MMAP(NULL,
> +		sizeof(int),
> +		PROT_READ | PROT_WRITE,
> +		MAP_SHARED | MAP_ANONYMOUS,
> +		-1, 0);
> +
> +	mq_freed2 = SAFE_MMAP(NULL,
> +		sizeof(int),
> +		PROT_READ | PROT_WRITE,
> +		MAP_SHARED | MAP_ANONYMOUS,
> +		-1, 0);

So here you are allocating two pages of memory for something that is
basically two bitflags. Can you at least change this to a single mmap()
something as:

static int *mq_freed;

	mq_freed = SAFE_MMAP(NULL, 2 * sizeof(int), ...)


	mq_freed[0] = 1;
	...

Moreover since we can actually stat()/access() the mqueue we can as well
check for the existence of the devdir in the cleanup and only remove it
if it exists in the filesystem.

Also I would be more afraid of the mqueue filesystem being mounted in
the temp directory if we trigger a failure between one of the
mount()/umount() calls, so we should as well check if it's mounted in
the cleanup and attempt to umount it.


-- 
Cyril Hrubis
chrubis@suse.cz

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