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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > 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.
> That's fine, but I followed suggestions in the reviews. I think that 
> having 3 people reviewing the same patch doesn't help the development 
> process. Now I'm not sure who I should follow :-)

It's actually the other way around, the more people look at the code the
better, at least that way we have potential to catch more problems
earlier. And if the reviewers disagree, let them fight for the right
answer.

I think that in this case this all can be actually simplified and we can
get rid of the mq_freed flag as I tried to outline below.

> >> -	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.
> >
> >
> Andrea
> 

-- 
Cyril Hrubis
chrubis@suse.cz

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