[new API] two-phased qb_ipcs_create required
jnpkrn opened this issue · 8 comments
Signatures
/* Creates a listening UNIX socket, akin to qb_ipcs_create */
qb_ipcs_listener_t* qb_ipcs_listener(const char *name);
/* Gets socket descriptor from listener; to prevent further
use (the same information is now at two places at minimum),
deallocates l, so _it must not be reused elsewhere from that
point on_ */
int qb_ipcs_listener_fd(const qb_ipcs_listener_t *l);
/* Wraps a listening UNIX socket, presumably created with
qb_ipcs_listener and subsequently extracted with qb_ipcs_listener_fd
(e.g. the name gets validated), extracting "name" using
getsockname(2); _fd must not be reused elsewhere from that point on_ */
qb_ipcs_listener_t* qb_ipcs_listener_from_fd(int fd);
/* Reuses existing UNIX socket wrapped in a listener object;
qb_ipcs_listener + qb_ipcs_create_from_listener equals qb_ipcs_create;
to prevent further use, deallocates l, so _it must not be reused
elsewhere from that point on_ */
qb_ipcs_service_t* qb_ipcs_create_from_listener(qb_ipcs_listener_t *l,
int32_t service_id,
enum qb_ipc_type type,
struct qb_ipcs_service_handlers *handlers);
Allow safe nesting wrt. NULL checking, e.g.,
qb_ipcs_create_from_listener(qb_ipcs_listener_from_fd(qb_ipcs_listener_fd(qb_ipcs_listener("foo"))),
42, QB_IPC_SHM, handlers);
The qb_ipcs_listener_t
abstraction is meant to allow for (and enforce)
extra checking on libqb side (so that the same internal coherency as with
monolithic qb_ipcs_create
is achieved), as that type is only used as
an opaque pointer in the client program.
Note that this would also (finally, one wants to say) offer client
programs to unset close-on-exec
flag for cases it would be counter
productive (what if pre-fork parent establishes the listening socket
only to be picked, with qb_ipcs_listener_from_fd
, by an after-fork
child with a brand new process image that was just exec'd?
btw. this looks exactly like a sequence every forking daemon
utilizing libqb's IPC subsystem should exercise unless parent-child
synchronization is in place -- it's highly undesirable for daemon
launcher process to indicate successful startup when the forked off
executive part fail on already occupied IPC channel the moment later!).
I'm currently split whether it should be a QB
branded flag that can
be passed to qb_ipcs_listener()
for convenience or whether authors
of client programs should be kept on their own with this.
Nonetheless, it must be documented that normally, this socket is
preconfigured with O_NONBLOCK
and FD_CLOEXEC
flags (respective to
particular fcntl
command). And that only FD_CLOEXEC
can reasonably
be mangled with, everything else asks for conflicting with intended
state of affairs on libqb side (but then, qb_ipcs_listener_from_fd()
can check or reset some vital flags on its own).
API looks sane, but what is the use case?
Also I was unable to understand following part:
btw. this looks exactly like a sequence every forking daemon
utilizing libqb's IPC subsystem should exercise unless parent-child
synchronization is in place -- it's highly undesirable for daemon
launcher process to indicate successful startup when the forked off
executive part fail on already occupied IPC channel the moment later!).
If it is about daemonization process, then why should daemon do the exec after fork?
I can imagine situation with child processes, but sharing fd is just asking for a big troubles.
On 27/09/18 00:18 -0700, Jan Friesse wrote: If it is about daemonization process, then why should daemon do the exec after fork?
In case it's a hierarchical daemon of daemons (e.g. pacemaker).
So it's not about daemonization.
I can imagine situation with child processes, but sharing fd is just asking for a big troubles.
Why? This is the same mechanism that, e.g., systemd uses to share the notification socket into services accommodating that signaling scheme.
Because current libqb IPC is not prepared for concurrent access.
Could you please explain the intended use case?
…
-- Jan (Poki)
Ok, now it makes a little more sense. I have doubts about the solution really solves any real problem (especially when daemon is systemd enabled and notifies its state), but as long as old api is kept, then why not.
Yes, it indeed does help with real use cases, details forthcoming.
This is not a hypothetical exercise if you think it is.
Have discovered that ability to force non-abstract sockets where they'd
otherwise get used (#248) -- likely together with pre-existing cases
where that had been already the case -- will break client programs that
run as non-root (making me wonder if that change was ever tested in
cluster stack context).
The main problem is that /var/run
is intentionally not world-writable,
meaning that bind(3) triggered with unprivileged process so as to lay
the socket there will effectively fail with EACCES
or similar
-- that's across the board of the OSes, all-permissive arrangement
yet to be spotted.
For pacemaker in particular, the solution is as "simple" as adopting
the solution per this request, as envisioned for other reasons already.
The master daemon of pacemaker (running as root, naturally) can then
get such sockets set up, especially for its non-root children, meaning
this particular problem would get solved along.
Any timeframe we can expect this?