On 2016-02-20 01:19, Ahmed S. Darwish wrote:
Hi!
On Tue, Feb 16, 2016 at 03:58:02PM +0100, David Henningsson wrote:
On 2016-02-12 01:19, Ahmed S. Darwish wrote:
Now that we have the necessary infrastructure for memexporting and
mempimporting a memfd memblock, extend that support higher up in the
chain with pstreams.
A PulseAudio endpoint can now _transparently_ send a memfd memblock
to the other end by simply calling pa_pstream_send_memblock().
...provided the memfds are first registered.
Yup, will add that.
...
@@ -92,6 +96,9 @@ struct item_info {
/* packet info */
pa_packet *packet;
+ bool shmid_to_memfd_packet:1;
This seems a bit strange to me. When you set up a srbchannel, there is a
special packet sent (PA_COMMAND_ENABLE_SRBCHANNEL) which has two fds as
ancil data.
Would it not make sense (and lead to less code changes, probably) if this
followed a similar scheme, i e, add a new command (PA_COMMAND_SHMID_FD or
something) that would have an memfd as ancil data and the ID as packet
content?
Hmm, I guess this should've been further commented.
Let's first agree to the following:
- to send a command like PA_COMMAND_ENABLE_SRBCHANNEL, a
tagstruct is built and sent over the pipe using
pa_pstream_send_tagstruct_with_fds()
[ protocol_native.c::setup_srbchannel() ]
- pa_pstream_send_tagstruct_with_fds() transform the tagstruct to
an opaque packet data using a simple memcpy(), then the packet
with its ancillary is sent over the domain socket.
[ pstream-util.c::pa_pstream_send_tagstruct_with_fds() ==>
pstream-util.c::pa_pstream_send_tagstruct_with_ancil_data()
==> pstream.c::pa_pstream_send_packet() /* event deferred */
==> pstream.c::do_write() /* packet write */
==> iochannel.c::pa_iochannel_write_with_fds()
==> POSIX sendmsg()! ]
So, _on the wire_, both srbchannel enablement and SHMID<->memfd
registration mechanisms are actually the same: the latter just
uses pa_pstream_send_packet() with extra little modifications.
Given the above, why not create a PA_COMMAND_SHMID_FD and save us
some code?
Well, the reason is that we want to close the memfd fd directly
after sending the packet and its ancillary using POSIX sendmsg().
pa_pstream_send_tagstruct_**() does not offer us any ability to
do that :-( It _opaques the message_ then defers the write event
over the pstream.
Due to such opaqueness, after a succesful pstream do_write() I
cannot just say: the packet written was a PA_COMMAND_SMID_FD, so
let's close its associated fds.
It seems quite straight forward just to add another parameter "bool
close_fds" to pa_pstream_send_tagstruct_with_fds and struct
pa_cmsg_ancil_data, and in pstream.c::do_write, right after the call to
pa_iochannel_write_with_fds, you close the fds if the parameter is set?
That way you don't need any "cleanup hook" either.
An argument could then be stated: why not wait for the other PA
endpoint to reply/ACK the command and then close the fds? Well,
this would open us to malicous clients .. leading to an fd leak
per each malicious client connection, and thus bringing the whole
server down after a while.
==
Thus the patch, actually simple, solution was created. Let's mark
the SHM_ID<->memfd registration packet with a special color. Now
in pstream.c::do_write() I can see that this is a registration
packet and call up a simple cleanup hook after write(). The hook
does all the necessary validations and then close the fds.
This way, the fds are always safely closed on the sending side.
[ After writing the above, I've just discovered a bug! If sendmsg
fails, the cleanup hook should also be called but it is not.
Will fix that ]
As a side note, we could potentially optimise setting up an srbchannel by
sending all three memfds in PA_COMMAND_ENABLE_SRBCHANNEL, it'll be one less
package to send...
I'm really not a big fan of this.. It will hardcode both the
number, _and nature_, of mempools in the Pulse communication
protocol itslef. Are you sure we want to do that?
Hey, it was just an idea :-)
Indeed that would mean some additional code, probably not worth it at
this point. There are just these really quick clients, e g if you do
"pactl list sinks" or something, and we're adding package after package
just to set the connection up. But thinking about it, this would
probably be better fixed if, e g, "pactl" could just disable SHM in the
first place.
So in the future, if we merge the per-client srbchannel mempool
with the envisioned per-client (earlier global) mempool, we will
have to change the protocol semantics. In the far future if we
reached the optimum case of a single mempool per client (not 3),
we'll also change the protocol semantics.
And in the present, we will have to insert lots of if conditions
and such if the client does not support memfds. Moreover, the fd
sending will be bidirectional. Three fds from the server if memfd
are supported [1 srbchannel, 1 srbchannel mempool, 1 global
mempool], and one fd from the client [audio data mempool]. And if
memfds are not supported, then 1 fd from the server and zero from
the client. This really complicates the protocol _a lot_ :-(
The current situation does not have this limitation at all.. Any
endpoint can create a shared mempool and send memblock references
to the other end. This preserves the dynamicism inherent in the
protocol and does not artificially limit it further.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
