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
