[pulseaudio-discuss] [PATCH 07/11] pulsecore: Introduce memfd support

[Tanu Kaskinen]

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