On Sun, 2015-09-20 at 23:33 +0200, Ahmed S. Darwish wrote:
> diff --git a/configure.ac b/configure.ac
> index 151e2b1..cee1bb4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -595,6 +595,23 @@ AC_DEFINE(HAVE_DLADDR, [1], [Have dladdr?])
>
> AM_ICONV
>
> +#### Linux memfd_create(2) SHM support ####
> +
> +AC_ARG_ENABLE([memfd],
> + AS_HELP_STRING([--disable-memfd], [Disable Linux memfd shared memory]))
> +
> +AS_IF([test "x$enable_memfd" != "xno"],
> + AC_CHECK_DECL(__NR_memfd_create, [HAVE_MEMFD=1], [HAVE_MEMFD=0], [#include ]),
I think SYS_memfd_create would be in some way more correct than
__NR_memfd_create. My manpage for syscall() uses SYS_readahead in an
example rather than __NR_readahead.
> diff --git a/src/pulsecore/memfd-wrappers.h b/src/pulsecore/memfd-wrappers.h
> new file mode 100644
> index 0000000..ef222ac
> --- /dev/null
> +++ b/src/pulsecore/memfd-wrappers.h
> @@ -0,0 +1,72 @@
> +#ifndef SRC_PULSECORE_MEMFD_WRAPPERS_H_
> +#define SRC_PULSECORE_MEMFD_WRAPPERS_H_
#define foomemfdwrappershfoo would be more consistent with other
headers.
> diff --git a/src/pulsecore/memfd.c b/src/pulsecore/memfd.c
> +static int __pa_memfd_create(pa_memfd *memfd, int fd, size_t size, bool writable) {
As I mentioned on another patch, it's not a good idea to use
identifiers with two underscores in the beginning, and the pa_ prefix
isn't nice here either. I suggest "memfd_create" as the function name.
> + int prot;
> +
> + memfd->fd = fd;
> + memfd->size = size;
> + prot = writable ? PROT_READ | PROT_WRITE : PROT_READ;
> +
> + memfd->ptr= mmap(NULL, size, prot, MAP_SHARED | MAP_NORESERVE, fd, 0);
Missing space before '='.
> +int pa_memfd_create(pa_memfd *memfd, size_t size) {
> + int fd;
> + pa_assert(memfd);
> + pa_assert(size > 0);
> + pa_assert(size <= MAX_SHARED_MEM_SIZE);
> +
> + if ((fd = memfd_create("memfd", MFD_ALLOW_SEALING)) < 0) {
MFD_CLOEXEC could be added to the flags. I don't actually really
remember what problems that avoids, and are those problems relevant
here, but I think PA sets that flag for pretty much all fds that it
opens. It shouldn't do any harm at least.
> diff --git a/src/pulsecore/memfd.h b/src/pulsecore/memfd.h
> +static inline bool PA_UNUSED pa_memfd_is_supported() {
> + return false;
> +}
Why is this marked as PA_UNUSED? Also, isn't the implementation too
simple (well, not this implementation, but the implementation when
HAVE_MEMFD is defined)? HAVE_MEMFD value depends on the build machine
kernel, but the build machine may be different than the runtime
machine, so HAVE_MEMFD may be defined, and still memfd isn't supported.
--
Tanu
