[PATCH] ctdb-common: Set close-on-exec when creating PID file (bug 12898)
Martin Schwenke
martin at meltin.net
Thu Jul 13 10:37:57 UTC 2017
On Thu, 13 Jul 2017 10:51:50 +0200, Volker Lendecke
<Volker.Lendecke at SerNet.DE> wrote:
> 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:
* API
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
(tridge) */
We should find a way of uniting them. Perhaps we can end up with:
* pidfile_path_create()
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.
* Leave pidfile_pid() pretty much how it is...
Thoughts?
peace & happiness,
martin
More information about the samba-technical
mailing list