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
