On Sat, 11 Oct 2025 at 21:20, Matthew Lugg <[email protected]> wrote: > > These tests cover the first two fixes in this patch series. The final > patch is not covered because the bug it fixes is not easily observable > by the guest. > > Signed-off-by: Matthew Lugg <[email protected]> > --- > tests/tcg/multiarch/test-mmap.c | 47 +++++++++++++++++++++++++++++---- > 1 file changed, 42 insertions(+), 5 deletions(-)
The test case itself looks good, so my review comments below are just minor nits. > diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c > index 96257f8ebe..64df694d1a 100644 > --- a/tests/tcg/multiarch/test-mmap.c > +++ b/tests/tcg/multiarch/test-mmap.c > @@ -22,6 +22,7 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > +#define _GNU_SOURCE Why do we need to define this now ? > #include <stdio.h> > #include <stdlib.h> > #include <stdint.h> > @@ -36,12 +37,12 @@ > do \ > { \ > if (!(x)) { \ > - fprintf(stderr, "FAILED at %s:%d\n", __FILE__, __LINE__); \ > + fprintf(stderr, " FAILED at %s:%d\n", __FILE__, __LINE__); \ I think that this is trying to fix a cosmetic bug in the printing of error messages: the tests each print some line without a newline, like: fprintf(stdout, "%s addr=%p", __func__, (void *)addr); and then for the passing case we add a space and complete the line: fprintf(stdout, " passed\n"); but this fail_unless() macro is not adding the space, so presumably we print something awkward like check_invalid_mmaps addr=0x12435468FAILED at ... But we should separate out this trivial cleanup from the patch adding a new test case. > exit (EXIT_FAILURE); \ > } \ > } while (0) > > -unsigned char *dummybuf; > +unsigned char *dummybuf; /* length is 2*pagesize */ > static unsigned int pagesize; > static unsigned int pagemask; > int test_fd; > @@ -439,21 +440,56 @@ void check_invalid_mmaps(void) > { > unsigned char *addr; > > + fprintf(stdout, "%s", __func__); > + > /* Attempt to map a zero length page. */ > addr = mmap(NULL, 0, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > - fprintf(stdout, "%s addr=%p", __func__, (void *)addr); > fail_unless(addr == MAP_FAILED); > fail_unless(errno == EINVAL); > > /* Attempt to map a over length page. */ > addr = mmap(NULL, -4, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > - fprintf(stdout, "%s addr=%p", __func__, (void *)addr); > fail_unless(addr == MAP_FAILED); > fail_unless(errno == ENOMEM); Can we leave the printfs for the existing test cases alone? You can add a new one for your new subcase: fprintf(stdout, "%s mremap addr=%p", __func__, addr); > + /* Attempt to remap a region which exceeds the bounds of memory. */ > + addr = mremap((void *)((uintptr_t)pagesize * 10), SIZE_MAX & > ~(size_t)pagemask, pagesize, 0); > + fail_unless(addr == MAP_FAILED); > + fail_unless(errno == EFAULT); > + > fprintf(stdout, " passed\n"); > } > > +void check_shrink_mmaps(void) > +{ > + unsigned char *a, *b, *c; > + a = mmap(NULL, pagesize * 2, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, > 0); > + b = mmap(NULL, pagesize * 2, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, > 0); > + c = mmap(NULL, pagesize * 2, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, > 0); > + > + fail_unless(a != MAP_FAILED); > + fail_unless(b != MAP_FAILED); > + fail_unless(c != MAP_FAILED); > + > + /* Ensure we can read the full mappings */ > + memcpy(dummybuf, a, 2 * pagesize); > + memcpy(dummybuf, b, 2 * pagesize); > + memcpy(dummybuf, c, 2 * pagesize); > + > + /* Shrink the middle mapping in-place; the others should be unaffected */ > + b = mremap(b, pagesize * 2, pagesize, 0); > + fail_unless(b != MAP_FAILED); > + > + /* Ensure we can still access all valid mappings */ > + memcpy(dummybuf, a, 2 * pagesize); > + memcpy(dummybuf, b, pagesize); > + memcpy(dummybuf, c, 2 * pagesize); > + > + munmap(a, 2 * pagesize); > + munmap(b, pagesize); > + munmap(c, 2 * pagesize); > +} > + > int main(int argc, char **argv) > { > char tempname[] = "/tmp/.cmmapXXXXXX"; > @@ -468,7 +504,7 @@ int main(int argc, char **argv) > > /* Assume pagesize is a power of two. */ > pagemask = pagesize - 1; > - dummybuf = malloc (pagesize); > + dummybuf = malloc (pagesize * 2); > printf ("pagesize=%u pagemask=%x\n", pagesize, pagemask); > > test_fd = mkstemp(tempname); > @@ -496,6 +532,7 @@ int main(int argc, char **argv) > check_file_fixed_eof_mmaps(); > check_file_unfixed_eof_mmaps(); > check_invalid_mmaps(); > + check_shrink_mmaps(); I was going to complain about indent on this line, but the problem seems to be that the file is incorrectly indented with hardcoded tabs for parts of it. > /* Fails at the moment. */ > /* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */ thanks -- PMM
