Re: [PATCH v2] selftest/vm: add mremap expand merge offset test

[David Hildenbrand]

On 02.01.23 16:49, Lorenzo Stoakes wrote:
> On Mon, Jan 02, 2023 at 04:34:14PM +0100, David Hildenbrand wrote:
> > On 02.01.23 15:44, Lorenzo Stoakes wrote:
> > > Add a test to assert that we can mremap() and expand a mapping starting
> > > from an offset within an existing mapping. We unmap the last page in a 3
> > > page mapping to ensure that the remap should always succeed, before
> > > remapping from the 2nd page.
> > >
> > > This is additionally a regression test for the issue solved in "mm, mremap:
> > > fix mremap() expanding vma with addr inside vma" and confirmed to fail
> > > prior to the change and pass after it.
> > >
> > > Finally, this patch updates the existing mremap expand merge test to check
> > > error conditions and reduce code duplication between the two tests.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> > > ---
> > >    tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++-----
> > >    1 file changed, 93 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
> >
> >
> > ...
> >
> > > +
> > > +	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> > > +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > > +
> > > +	if (start == MAP_FAILED) {
> > > +		ksft_print_msg("mmap failed: %s\n", strerror(errno));
> >
> > I'd
> >
> > 	ksft_test_result_fail(...)
> > 	return;
> >
> > > +		goto out;
> > > +	}
> > > +
> > > +	munmap(start + page_size, page_size);
> > > +	remap = mremap(start, page_size, 2 * page_size, 0);
> > > +	if (remap == MAP_FAILED) {
> > > +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
> > > +		munmap(start, page_size);
> > > +		munmap(start + 2 * page_size, page_size);
> > > +		goto out;
> >
> > dito
> >
> > 	ksft_test_result_fail(...)
> > 	...
> > 	return;
> >
> > > +	}
> > > +
> > > +	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
> > > +	munmap(start, 3 * page_size);
> > > +
> > > +out:
> >
> > then you can drop the out label.
>
> I have to disagree on this, to be consistent with the other tests the
> failure messages should include the test name, and putting the
> ksft_test_result_fail("test name") in each branch as well as the error
> message would just be wilful duplication.
>
> I do think it's a pity C lacks mechanisms such that gotos are sometimes
> necessary, but I can only right so many wrongs in this patch :)

Let's agree to disagree ;) Too bad we don't have prefix push/pop 
functionality as we have in other similar testing frameworks -- to avoid 
exactly that duplication.

[...]

> > I'd simply use a global variable, same applies for page_size. But passing it
> > around is also ok.
>
> I am trying to keep things consistent with what's gone before in this code,
> and given page_size is being passed around I think the 'when in Rome'
> principle applies equally to passing the fp around I think.

Other tests we have handle it "easier". :)

--
Thanks,

David / dhildenb