[pulseaudio-discuss] [PATCH 10/11] pulsecore: pstreams: Introduce memfd blocks support
Tanu Kaskinen
tanuk at iki.fi
Wed Sep 30 02:05:26 PDT 2015
On Sun, 2015-09-20 at 23:38 +0200, Ahmed S. Darwish wrote:
> diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> index 6e1963f..a65b0a6 100644
> --- a/src/pulsecore/pstream.c
> +++ b/src/pulsecore/pstream.c
> @@ -46,6 +46,7 @@
> #define PA_FLAG_SHMDATA 0x80000000LU
> #define PA_FLAG_SHMRELEASE 0x40000000LU
> #define PA_FLAG_SHMREVOKE 0xC0000000LU
> +#define PA_FLAG_MEMFD_FD 0x01000000LU
"PA_FLAG_MEMFD_FD" sounds like only the fd is being sent, while in
reality this is analogous to PA_FLAG_SHMDATA. Maybe PA_FLAG_MEMFDDATA
would be a good name.
If you do what David suggested, though, I'm not sure a new flag is even
needed.
> @@ -527,10 +568,6 @@ static void prepare_next_write_item(pa_pstream *p) {
> flags = (uint32_t) (p->write.current->seek_mode & PA_FLAG_SEEKMASK);
>
> if (p->use_shm) {
Note that here we only check whether some kind of shared memory has
been negotiated with the other end...
> - uint32_t block_id, shm_id;
> - size_t offset, length;
> - uint32_t *shm_info = (uint32_t *) &p->write.minibuf[PA_PSTREAM_DESCRIPTOR_SIZE];
> - size_t shm_size = sizeof(uint32_t) * PA_PSTREAM_SHM_MAX;
> pa_mempool *current_pool = pa_memblock_get_pool(p->write.current->chunk.memblock);
> pa_memexport *current_export;
>
> @@ -539,28 +576,70 @@ static void prepare_next_write_item(pa_pstream *p) {
> else
> pa_assert_se(current_export = pa_memexport_new(current_pool, memexport_revoke_cb, p));
>
> - if (pa_memexport_shm_put(current_export,
> - p->write.current->chunk.memblock,
> - &block_id,
> - &shm_id,
> - &offset,
> - &length) >= 0) {
> + if (p->write.current->type == PA_PSTREAM_ITEM_MEMBLOCK_SHARED_POSIX) {
> + uint32_t block_id, shm_id;
> + size_t offset, length;
> + uint32_t *shm_info = (uint32_t *) &p->write.minibuf[PA_PSTREAM_DESCRIPTOR_SIZE];
> + size_t shm_size = sizeof(uint32_t) * PA_PSTREAM_SHM_MAX;
> +
> + memexport_result = pa_memexport_shm_put(current_export,
> + p->write.current->chunk.memblock,
> + &block_id,
> + &shm_id,
> + &offset,
> + &length);
> +
> + if (memexport_result >= 0) {
> + flags |= PA_FLAG_SHMDATA;
> +
> + shm_info[PA_PSTREAM_SHM_BLOCKID] = htonl(block_id);
> + shm_info[PA_PSTREAM_SHM_SHMID] = htonl(shm_id);
> + shm_info[PA_PSTREAM_SHM_INDEX] = htonl((uint32_t) (offset + p->write.current->chunk.index));
> + shm_info[PA_PSTREAM_SHM_LENGTH] = htonl((uint32_t) p->write.current->chunk.length);
> +
> + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] = htonl(shm_size);
> + p->write.minibuf_validsize = PA_PSTREAM_DESCRIPTOR_SIZE + shm_size;
> + }
> +
> + } else if (p->write.current->type == PA_PSTREAM_ITEM_MEMBLOCK_SHARED_MEMFD) {
...but here we don't check that we have negotiated memfd-based shared
memory. The exported memblock may come from some other mempool than
this pstream's own mempool.
For example, we may have two clients that use different versions of
libpulse (sandboxing makes this somewhat plausible), one of which is
built with memfd support and the other isn't. I'm not sure that it's
possible that a memblock from one client's mempool ends up being sent
to another client, but I don't have proof that it can't happen.
> + uint32_t block_id;
> + int memfd_fd = -1;
> + size_t offset, length;
> + uint32_t *memfd_info = (uint32_t *) &p->write.minibuf[PA_PSTREAM_DESCRIPTOR_SIZE];
> + size_t memfd_size = sizeof(uint32_t) * PA_PSTREAM_MEMFD_MAX;
> +
> + memexport_result = pa_memexport_memfd_put(current_export,
> + p->write.current->chunk.memblock,
> + &block_id,
> + &memfd_fd,
> + &offset,
> + &length);
> +
> + if (memexport_result >= 0) {
> + flags |= PA_FLAG_MEMFD_FD;
> +
> + memfd_info[PA_PSTREAM_MEMFD_BLOCKID] = htonl(block_id);
> + memfd_info[PA_PSTREAM_MEMFD_INDEX] = htonl((uint32_t) (offset + p->write.current->chunk.index));
> + memfd_info[PA_PSTREAM_MEMFD_LENGTH] = htonl((uint32_t) p->write.current->chunk.length);
> +
> + pa_assert(memfd_fd >= 0);
> + p->write.current->with_ancil_data = true;
> + p->write.current->ancil_data.creds_valid = false;
> + p->write.current->ancil_data.nfd = 1;
> + p->write.current->ancil_data.fds[0] = memfd_fd;
This won't compile if HAVE_CREDS is not defined, because ancil_data
doesn't exist.
> @@ -874,30 +961,54 @@ static int do_read(pa_pstream *p, struct pstream_read *re) {
>
> pa_packet_unref(re->packet);
> } else {
> - pa_memblock *b;
> + pa_memblock *b = NULL;
> uint32_t flags = ntohl(re->descriptor[PA_PSTREAM_DESCRIPTOR_FLAGS]);
> - pa_assert((flags & PA_FLAG_SHMMASK) == PA_FLAG_SHMDATA);
> -
> pa_assert(p->import);
>
> - if (!(b = pa_memimport_shm_get(p->import,
> - ntohl(re->shm_info[PA_PSTREAM_SHM_BLOCKID]),
> - ntohl(re->shm_info[PA_PSTREAM_SHM_SHMID]),
> - ntohl(re->shm_info[PA_PSTREAM_SHM_INDEX]),
> - ntohl(re->shm_info[PA_PSTREAM_SHM_LENGTH]),
> - !!(flags & PA_FLAG_SHMWRITABLE)))) {
> -
> - if (pa_log_ratelimit(PA_LOG_DEBUG))
> - pa_log_debug("Failed to import memory block.");
> + if ((flags & PA_FLAG_SHMMASK) == PA_FLAG_SHMDATA) {
> + b = pa_memimport_shm_get(p->import,
> + ntohl(re->per_type.shm_info[PA_PSTREAM_SHM_BLOCKID]),
> + ntohl(re->per_type.shm_info[PA_PSTREAM_SHM_SHMID]),
> + ntohl(re->per_type.shm_info[PA_PSTREAM_SHM_INDEX]),
> + ntohl(re->per_type.shm_info[PA_PSTREAM_SHM_LENGTH]),
> + !!(flags & PA_FLAG_SHMWRITABLE));
> + } else if ((flags & PA_FLAG_SHMMASK) == PA_FLAG_MEMFD_FD) {
> + int memfd_fd;
> + pa_assert(p->read_ancil_data.nfd == 1);
What if the other end sends a wrong number of fds? Is that checked
before this line? If not, the assertion should be replaced with proper
error handling.
> +
> + memfd_fd = p->read_ancil_data.fds[0];
> + pa_assert(memfd_fd >= 0);
> +
> + b = pa_memimport_memfd_get(p->import,
> + ntohl(re->per_type.memfd_info[PA_PSTREAM_MEMFD_BLOCKID]),
> + memfd_fd,
> + ntohl(re->per_type.memfd_info[PA_PSTREAM_MEMFD_INDEX]),
> + ntohl(re->per_type.memfd_info[PA_PSTREAM_MEMFD_LENGTH]),
> + !!(flags & PA_FLAG_SHMWRITABLE));
> + } else {
> + pa_assert_not_reached();
> }
>
> + if (!b && pa_log_ratelimit(PA_LOG_DEBUG))
> + pa_log_debug("Failed to import memory block.");
> +
> if (p->receive_memblock_callback) {
> int64_t offset;
> pa_memchunk chunk;
>
> chunk.memblock = b;
> chunk.index = 0;
> - chunk.length = b ? pa_memblock_get_length(b) : ntohl(re->shm_info[PA_PSTREAM_SHM_LENGTH]);
> +
> + if (b) {
> + chunk.length = pa_memblock_get_length(b);
> + } else {
> + if ((flags & PA_FLAG_SHMMASK) == PA_FLAG_SHMDATA)
> + chunk.length = ntohl(re->per_type.shm_info[PA_PSTREAM_SHM_LENGTH]);
> + else if ((flags & PA_FLAG_SHMMASK) == PA_FLAG_MEMFD_FD) {
> + chunk.length = ntohl(re->per_type.memfd_info[PA_PSTREAM_MEMFD_LENGTH]);
> + } else
Redundant curly brances.
--
Tanu
More information about the pulseaudio-discuss
mailing list