[pulseaudio-discuss] [PATCH v2 05/12] pulsecore: SHM: Introduce memfd support

[Ahmed S. Darwish]

On Tue, Feb 16, 2016 at 02:26:31PM +0100, David Henningsson wrote:
> 
> On 2016-02-13 02:58, Ahmed S. Darwish wrote:
> >
> >On Fri, Feb 12, 2016 at 04:49:23PM +0100, David Henningsson wrote:
> >>
> >>On 2016-02-12 01:14, Ahmed S. Darwish wrote:
> >>>+    /* Do not close file descriptors which are not our own */
> >>>+    if (type != PA_MEM_TYPE_SHARED_MEMFD)
> >>>+        pa_assert_se(pa_close(fd) == 0);
> >>
> >>I believe the fd should be closed regardless of type?
> >>
> >>Now you seem to leak an fd if type == PA_MEM_TYPE_SHARED_MEMFD?
> >>
> >
> >Hmm, that comment should've been further clarified..
> >
> >By "not close file descriptors which are not our own", I meant
> >not to close the fd behind our caller's back. It was just passed
> >to us as a parameter and thus we do not have any kind of
> >ownership to close it.
> >
> >This is unlike attaching to a Posix SHM  where the fd was created
> >by us in the same code path [ using shm_open() ] and thus we have
> >its complete ownership -- and must close it afterwards.
> >
> >So for memfd attachment, the caller owns the passed fd and is
> >responsible for closing it when it sees fit. That's also why we
> >do not cache the passed fd in the callee and set pa_shm.memfd.fd
> >to -1.
> 
> Right, so if you would have instead written something like:
> 
> if (type == PA_MEM_TYPE_SHARED_MEMFD)
>     m->per_type.memfd.fd = fd;
> else
>     pa_assert_se(pa_close(fd) == 0);
> 
> ...it would be more obvious that the ownership of the fd is
> transferred into the struct at that point, or closed.
> 

ACK..

It's now like the above in v3 for pa_shm_create().

For pa_shm_attach(), the fd is never cached anyway and is always
set to -1, so a more verbose comment was just added.

Thanks,

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