[pulseaudio-discuss] [PATCH v2 06/12] pulsecore: memexport/memimport: Support memfd blocks

[Ahmed S. Darwish]

Hi :-)

On Tue, Feb 23, 2016 at 01:09:17PM +0100, David Henningsson wrote:
> On 2016-02-19 20:28, Ahmed S. Darwish wrote:
> >
...
> >
> >Anyway here's how shm_attch() now looks like in v3:
> >
> >   static int shm_attach(pa_shm *m, pa_mem_type_t type, unsigned
> >                         id, int memfd_fd, ..) {
> >       ..
> >       /* In case of attaching to memfd areas, _the caller_
> >        * maintains ownership of the passed fd and has the sole
> >        * responsibility of closing it down.. For other types, we
> >        * are the code path  which created the fd in the first
> >        * place and we're thus the ones responsible for closing it
> >        * down */
> >       if (type != PA_MEM_TYPE_SHARED_MEMFD)
> >           pa_assert_se(pa_close(fd) == 0);
> >
> >       m->type = type;
> >       m->id = id;
> >       m->size = (size_t) st.st_size;
> >       m->do_unlink = false;
> >       m->fd = -1;
> >   }
> >
> >I guess this is now much more clear :-)
> 
> Okay, so it's mostly a design decision then?

Yup, exactly..

> Whether to take ownership of the memfd and thus close it down
> when we're done (and simpler code as a result?), or let the
> caller do the same right after the call to shm_attach?
>

Making the caller owns the fd is actually simpler ;-)

Here's the sequence of receiving a memfd fd:

  packet_complete() - fd received from pipe
    -> pa_memimport_attach_memfd()
      -> segment_attach()
        -> pa_shm_attach()

If we let pa_shm_attach() owns the passed fd, then we'll have to
sprinkle extra 'pa_close(fd)' instances in the sequence above :-(

Why? cause sometimes one method in the chain fails before callig
its lower one. For example, segment_attach() directly quits if
number of attached segments exceeds PA_MEMIMPORT_SEGMENTS_MAX –
pa_shm_attach() is not even reached.

Meanwhile, making the caller owns the fd will let us localize the
pa_close(fd) in just one place: inside packet_complete() itself.

Thanks! (for your always thought-provoking reviews)

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