On Wed, 27 Feb 2019 21:13:10 +0200 Leonid Bobrov <mazoc...@disroot.org> wrote:
> From: Imre Vadász <ivad...@dragonflybsd.org> > > Signed-off-by: Leonid Bobrov <mazoc...@disroot.org> > --- > configure.ac | 4 ++++ > src/wayland-shm.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+) Hi Leonid, this is almost a good patch. I'd probably like the MAP_ANON thing as a separate patch though, since it doesn't seem to be related to the mremap usage. More below. > > diff --git a/configure.ac b/configure.ac > index c0f1c37..dcefc78 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -65,6 +65,10 @@ AC_SUBST(GCC_CFLAGS) > AC_CHECK_HEADERS([sys/prctl.h]) > AC_CHECK_FUNCS([accept4 mkostemp posix_fallocate prctl]) > > +# mremap() and sys/mman.h are needed for the shm > +AC_CHECK_FUNCS([mremap]) > +AC_CHECK_HEADERS([sys/mman.h]) > + > # waitid() and signal.h are needed for the test suite. > AC_CHECK_FUNCS([waitid]) > AC_CHECK_HEADERS([signal.h]) > diff --git a/src/wayland-shm.c b/src/wayland-shm.c > index 4191231..5bfdece 100644 > --- a/src/wayland-shm.c > +++ b/src/wayland-shm.c > @@ -40,6 +40,9 @@ sys/mman.h is checked in configure.ac, but the result is not used. > #include <assert.h> > #include <signal.h> > #include <pthread.h> > +#ifdef HAVE_SYS_PARAM_H Was there something to set this in configure.ac? I didn't see it. > +#include <sys/param.h> > +#endif > > #include "wayland-util.h" > #include "wayland-private.h" > @@ -84,7 +87,24 @@ shm_pool_finish_resize(struct wl_shm_pool *pool) > if (pool->size == pool->new_size) > return; > > +#ifdef HAVE_MREMAP > data = mremap(pool->data, pool->size, pool->new_size, MREMAP_MAYMOVE); > +#else > + int32_t osize = (pool->size + PAGE_SIZE - 1) & ~PAGE_MASK; We try to not mix code and declarations, i.e. osize would need to be declared at the start of the function or at least at the start of a block. > + if (pool->new_size <= osize) { > + pool->size = pool->new_size; > + return; > + } > + data = mmap(pool->data + osize, pool->new_size - osize, PROT_READ, > + MAP_SHARED | MAP_TRYFIXED, pool->fd, osize); > + if (data == MAP_FAILED) { > + munmap(pool->data, pool->size); > + data = mmap(NULL, pool->new_size, PROT_READ, MAP_SHARED, > pool->fd, 0); > + } else { > + pool->size = pool->new_size; > + return; > + } Because we already reject shrinking in shm_pool_resize(), this implementation looks correct to me. > +#endif > if (data == MAP_FAILED) { The failure path here seems to leave the old munmapped address in pool->data which would cause it to be munmapped again when the pool is destroyed. I also don't think munmap(MAP_FAILED, ...) would be a no-op, so would need to protect against that as well. > wl_resource_post_error(pool->resource, > WL_SHM_ERROR_INVALID_FD, > @@ -495,6 +515,7 @@ sigbus_handler(int signum, siginfo_t *info, void *context) > sigbus_data->fallback_mapping_used = 1; > > /* This should replace the previous mapping */ > +#ifdef HAVE_MREMAP > if (mmap(pool->data, pool->size, > PROT_READ | PROT_WRITE, > MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, > @@ -502,6 +523,15 @@ sigbus_handler(int signum, siginfo_t *info, void > *context) > reraise_sigbus(); > return; > } > +#else > + if (mmap(pool->data, pool->size, > + PROT_READ, > + MAP_PRIVATE | MAP_FIXED | MAP_ANON, > + 0, 0) == MAP_FAILED) { > + reraise_sigbus(); > + return; > + } > +#endif The only difference here seems to be MAP_ANONYMOUS vs. MAP_ANON. Why would that be related to the existence of mremap()? My Linux manual says that MAP_ANON is a deprecated synonym for MAP_ANONYMOUS. How about #ifndef MAP_ANONYMOUS #define MAP_ANONYMOUS MAP_ANON #endif instead of this hunk? Thanks, pq > } > > static void
pgpKc3vG61tmZ.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel