Re: [PATCH] ctdb-common: Set close-on-exec when creating PID file (bug 12898)
On Thu, Jul 13, 2017 at 4:21 PM, Martin Schwenke via samba-technical <
[hidden email]> wrote:
> Otherwise, for example, the file descriptor for the main PID file will
> leak all the way down to event scripts.
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12898 >
> Please review and maybe push...
> peace & happiness,
> On Thu, Jul 13, 2017 at 04:21:19PM +1000, Martin Schwenke via samba-technical wrote:
> > Otherwise, for example, the file descriptor for the main PID file will
> > leak all the way down to event scripts.
> What prevents ctdb and the rest of samba to use a common pidfile
> handling? The version in lib/util already has that close on exec bit
> set, at least it does have a call to smb_set_close_on_exec(fd);.
Yes, that is definitely a good goal. Amitay put together the CTDB
version because we had a bug that we were fixing under time pressure.
We decided not to solve the issues at that time...
There are a few things:
We really just want a file pathname instead of directory and basename.
The main reason for this is that initscripts and systemd unit files
(and. consequently, ctdbd_wrapper) need to know the full pathname, so
it doesn't make sense to split it and re-join it.
* Coverity hit
lib/util/pidfile.c currently leaks the file descriptor and Coverity
notices. Amitay's implementation stores it in a context. When the
context is freed then the destructor does unlink/unlock/close.
* Fear ;-)
lib/util/pidfile.c has this at the top:
/* this code is broken - there is a race condition with the unlink
We should find a way of uniting them. Perhaps we can end up with:
Renamed from ctdb/common/pidfile.c pidfile_create()
* Wrap pidfile_create() around pidfile_path_create()
* Drop pidfile_unlink() in favour of using struct pidfile_context and
talloc_free()ing it to unlink/unlock/close.