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
