Re: [PATCH] Introduce xcb_aux_alloc_shm() to create anonymous files in RAM

13.04.2018 09:47, Uli Schlachter пишет:
>> +[],
>> +[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?)

> 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.

Well, I just copied it from 
https://cgit.freedesktop.org/xorg/lib/libxshmfence/tree/configure.ac
The same code is used in the X server: 
https://cgit.freedesktop.org/xorg/xserver/tree/configure.ac#n1121
Somehow it's working there...

> 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?
memfd_create() was introduced only in the latest glibc 2.27,
there are many systems where it's not available yet.
I'd prefer not to drop the wrapper.

>> +
>> +#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?
I copied this from 
https://cgit.freedesktop.org/xorg/lib/libxshmfence/tree/src/xshmfence_alloc.c
Yes, it seems to be better to make sys/memfd.h required.

> Also, why MFD_ALLOW_SEALING?
>
> [...]
>> +int
>> +xcb_aux_alloc_shm(size_t size)
> Should the documentation mention anything about sealing? close-on-exec?
Yes, it is worth mentioning them.
Besides the size of the file should be fixed with F_ADD_SEALS.

>> +{
>> +	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?
Agree.
_______________________________________________
Xcb mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/xcb