[LTP] [PATCH v7 3/6] Refactor mqns_03 using new LTP API
Andrea Cervesato
andrea.cervesato@suse.com
Wed Jul 5 15:16:18 CEST 2023
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
More information about the ltp
mailing list