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

[Andrea Cervesato]

Hi Cyril,

On 5/22/23 16:41, Cyril Hrubis wrote:
> 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.
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 :-)
>> -	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