[pulseaudio-discuss] [PATCH v2 09/12] pulsecore: pstream: Support memfd blocks transport

[Ahmed S. Darwish]

Hi!

On Tue, Feb 23, 2016 at 09:32:44PM +0200, Ahmed S. Darwish wrote:
> 
> On Tue, Feb 23, 2016 at 01:34:41PM +0100, David Henningsson wrote:
> > 
> > On 2016-02-20 01:19, Ahmed S. Darwish wrote:
> > >
> ...
> > >
> > >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.
> >
> 
> That's infinitely better, and honestly what should've been done
> in the first place..
> 
> Will now update v3 to use a PA_COMMAND and this form of direct,
> parameter-based, cleanup.
>

This was too qucik a reply ;-) While coding v3 I've just
remembered the second reason for choosing this custom packet
approach .. which admittedly should've been better documented.

Unfortunately the fds cannot just be directly closed at the
pa_pstream_send_tagstruct_with_fds() method itself. That would
create a race condition where the fds might be closed before ever
being passed to the other side :-(

Why? Such a method just defers packet sending over the pipe and
never does the actual sending by itself. It internally calls
pa_pstream_send_packet() which does:

  i = pa_xnew(struct item_info, 1);
  ...
  pa_queue_push(p->send_queue, i);
  p->mainloop->defer_enable(p->defer_event, 1);

So we'll need to handle that race: as was done in the original
submission, the fd should be closed only after doing the actual
sending in pstream.c do_write().

==

Nonetheless, I still agree that a new packet type is possibly an
overkill. So let's mix both solutions in a hopefully optimal
manner:

  1. pa_pstream_send_tagstruct_with_fds() will gain the extra
     'close_fds' boolean as advised

  2. pa_cmsg_ancil_ancil_data will get the same extra 'close_fds'
     boolean. send_tagstruct() at point #1 will just copy that
     boolean to the ancil data

  3. pstream.c do_write(), which does actual writing, will
     inspect the ancillary data 'close_fds' variable and close
     the fds if instructed – after completing all the necessary
     sendmsg(2) system calls.

What are your thoughts on the above?

Thanks!

-- 
Darwish
http://darwish.chasingpointers.com