Re: [LTP] [PATCH v4] listmount04.c: Update case support mnt_id_req.mnt_ns_fd

[Petr Vorel <pvorel@suse.cz>]

...
> > @@ -122,7 +136,17 @@ static void run(unsigned int n)
> >  		req->mnt_id = tc->mnt_id;
> >  		req->param = tc->param;
> >  		req->size = tc->size;
> > -		req->spare = tc->spare;
> > +#ifdef HAVE_STRUCT_MNT_ID_REQ_MNT_NS_FD
> > +		if ((tst_kvercmp(6, 18, 0)) >= 0)
> > +			req->mnt_ns_fd = tc->mnt_ns_fd;
> > +		else
> > +			tst_brk(TCONF, "Skipping test, kernel version should be >= 6.18");
> > +#else
> > +		if ((tst_kvercmp(6, 18, 0)) >= 0)
> > +			tst_brk(TCONF, "Skipping test, kernel version should be < 6.18");
> > +		else
> > +			req->spare = tc->spare;
> > +#endif

> I do not like this solution. Here we disable the test on newer kernel if
> it was compiled on older headers and vice versa.

Thanks for catching this up. I had a feeling something is wrong but was not sure
what.

> There are actually two problems to be solved and they are independent of
> each other and we shouldn't mix these two.

> First problem is that the mnt_ns_fd is not defined on older headers.
> That should be fixed by a fallback defintion in lapi/. The spare in
> lapi/mount.h in struct mnt_id_req should be renamed to mnt_ns_fd. And we
> should use that structure if mnt_id_req is not defined in system
> headers. I guess that we can use typedef for that and do something as:

OK, I did not realize that struct size is actually the same, only struct member
has changed so that we can happily pass that to any kernel version.

> struct mnt_id_req_fallback {
> 	uint32_t size;
> 	...
> };

> #if !defined(HAVE_STRUCT_MNT_ID_REQ) || !HAVE_STRUCT_MNT_ID_REQ_MNT_NS_FD
> typedef struct mnt_id_req_fallback mnt_id_req
> #else
> typedef struct mnd_id_req mnt_id_req
> #endif

> And then use mnt_id_req in test with the guarantee that the mnt_ns_fd is
> always there.

> The second problem is the expected errno. That should be just set once
> based on the kernel version in the test setup().

+1. I was also thinking about moving this to setup to prevent repeated detecting
but was not sure how to do it in elegant way as errno is in the struct. I guess
you mean just to store result of tst_kvercmp(6, 18, 0), but even that is not
enough. The cleanest way looks to me to add another struct member int exp_errno_old:

static bool old_kernel = true;

static struct tcase {
	...
	int exp_errno;
	int exp_errno_old;
}

static void setup()
{
	if ((tst_kvercmp(6, 18, 0)) < 0)
		old_kernel = false;
}

static void run(unsigned int n)
{
	...
	int exp_errno = (tc->exp_errno_old && old_kernel ? tc->exp_errno_old : tc->exp_errno);

	TST_EXP_FAIL(tst_syscall(__NR_listmount, req, tc->mnt_ids,
		tc->nr_mnt_ids, tc->flags), exp_errno,
		"%s", tc->msg);
	}


Also when looking in the code I'd personally factor out tc->req_usage to make it
obvious	it's not called by first tcases member.

Kind regards,
Petr

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