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

[David Hildenbrand <david@redhat.com>]

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@gmail.com>
>>> ---
>>>    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