On 10.04.2018 13:38, Alexander Volkov wrote:
> ... which then can be mapped with mmap().
> It is intended to be used in conjunction with xcb_shm_attach_fd()
> to access the created shared memory from a client and the X server.
>
> Signed-off-by: Alexander Volkov <a.vol...@rusbitech.ru>
> ---
> configure.ac | 47 +++++++++++++++++++++++++++++
> src/xcb_aux.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
> src/xcb_aux.h | 3 ++
> 3 files changed, 133 insertions(+)
>
> diff --git a/configure.ac b/configure.ac
> index 1fe1561..81e1f6b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -7,10 +7,57 @@ AC_CONFIG_HEADERS([config.h])
> AC_CONFIG_MACRO_DIR([m4])
> AM_INIT_AUTOMAKE([foreign dist-bzip2])
>
> +AC_USE_SYSTEM_EXTENSIONS
> +
> XCB_UTIL_COMMON([1.4], [1.6])
>
> AC_CHECK_FUNCS_ONCE(vasprintf)
>
> +AC_CHECK_FUNCS(memfd_create mkostemp)
> +
> +AC_CHECK_DECLS([__NR_memfd_create], [], [], [[#include <asm/unistd.h>]])
> +
> +AC_CHECK_HEADERS([sys/memfd.h], [AC_DEFINE([HAVE_MEMFD_H], 1, [Has
> sys/memfd.h header])])
> +
> +AC_ARG_WITH(shared-memory-dir,
> AS_HELP_STRING([--with-shared-memory-dir=PATH], [Path to directory in a
> world-writable temporary directory for anonymous shared memory (default:
> auto)]),
Default is yes, not auto. See next two lines.
> +[],
> +[with_shared_memory_dir=yes])
> +
> +shmdirs="/run/shm /dev/shm /var/tmp /tmp"
> +
> +case x"$with_shared_memory_dir" in
> +xyes)
> + for dir in $shmdirs; do
> + case x"$with_shared_memory_dir" in
> + xyes)
How many shells are there that do not like "break"? :-/
(And does autoconf provide some way to make this loop nicer?)
> + echo Checking temp dir "$dir"
> + if test -d "$dir"; then
> + with_shared_memory_dir="$dir"
> + fi
> + ;;
> + esac
> + done
> + ;;
> +x/*)
> + ;;
> +xno)
> + ;;
> +*)
> + AC_MSG_ERROR([Invalid directory specified for --with-shared-memory-dir:
> $with_shared_memory_dir])
> + ;;
> +esac
> +
> +case x"$with_shared_memory_dir" in
> +xyes)
> + AC_MSG_ERROR([No directory found for shared memory temp files.])
> + ;;
> +xno)
> + ;;
> +*)
> + AC_DEFINE_UNQUOTED(SHMDIR, ["$with_shared_memory_dir"], [Directory for
> shared memory temp files])
> + ;;
> +esac
> +
> AC_CONFIG_FILES([Makefile
> src/Makefile
> xcb-atom.pc
So, if I do --with-shared-memory-dir=/foobar, configure will tell me
that I specified an invalid directory? Why?? And why does this flag
exist at all, i.e. what would be uses for it?
Also, why does the existence of a directory at compile time have any
significance at run time? What about cross compiling? Building a distro
package with /run/shm existing and then running it on another system
that does not have /run/shm?
I really think that this compile time check is plain wrong.
Also, is it really worth it to have all these different cases?
- Linux with updated glibc that provides a memfd_create() wrapper
- Linux with older glibc where this code provides its own wrapper
- Older Linux with memfd_create() (-> mkostemp())
- Anything else (-> mkstemp())
How about dropping the second one (own syscall wrapper if glibc does not
provide one). Oh and how about making sys/memfd.h required instead of
providing the defines here instead, if needed.
What would be the downside of this? How many people would complain?
> diff --git a/src/xcb_aux.c b/src/xcb_aux.c
> index b6f64f8..e8f7ba9 100644
> --- a/src/xcb_aux.c
> +++ b/src/xcb_aux.c
> @@ -33,9 +33,39 @@
> #include "config.h"
> #endif
>
> +#if !HAVE_MEMFD_CREATE
> +#if HAVE_DECL___NR_MEMFD_CREATE
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +static int memfd_create(const char *name,
> + unsigned int flags)
> +{
> + return syscall(__NR_memfd_create, name, flags);
> +}
> +#define HAVE_MEMFD_CREATE 1
> +#endif
> +#endif
> +
> +#if HAVE_MEMFD_CREATE
> +
> +/* Get defines for the memfd_create syscall, using the
> + * header if available, or just defining the constants otherwise
> + */
> +
> +#if HAVE_MEMFD_H
> +#include <sys/memfd.h>
> +#else
> +/* flags for memfd_create(2) (unsigned int) */
> +#define MFD_CLOEXEC 0x0001U
> +#define MFD_ALLOW_SEALING 0x0002U
> +#endif
This looks quite linux-specific. Is it worth adding a linux-specific #if
in here in case any other platform ever implements this API as well and
assigns flags differently?
Also, why MFD_ALLOW_SEALING?
[...]
> +int
> +xcb_aux_alloc_shm(size_t size)
Should the documentation mention anything about sealing? close-on-exec?
> +{
> + char template[] = SHMDIR "/shmfd-XXXXXX";
This uses shmfd, but...
> + int fd;
> +
> +#if HAVE_MEMFD_CREATE
> + fd = memfd_create("xcb_aux", MFD_CLOEXEC|MFD_ALLOW_SEALING);
[...]
...this uses xcb_aux.
Shouldn't they best use the same template name?
Cheers,
Uli
--
Happiness can't be found -- it finds you.
- Majic
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
