[pve-devel] [PATCH cluster] pmxcfs: only exit parent when successfully started

[Fabian Grünbichler]

minor nits, ACK otherwise

On Thu, Apr 12, 2018 at 11:27:09AM +0200, Dominik Csapak wrote:
> since systemd depends that the pid file is written only
> when the service is actually started, we need to wait for the
> child to get to the point where it starts the fuse loop
> and signal the parent to now exit and write the pid file

nit: it's not the PID file that systemd cares about, but the exit of the
main process. the PID file just helps systemd keep track of the service
without needing to guess which process is "the service".

> 
> without this, we had an issue, where the
> ExecStartPost hook (which runs pvecm updatecerts) did not run reliably,

and any other service with an "After=pve-cluster", which could have run
into this sporadically as well.

> but which is necessary to setup the nodes/ dir in /etc/pve
> and generating the ssl certificates
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  data/src/pmxcfs.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/data/src/pmxcfs.c b/data/src/pmxcfs.c
> index 6047ad0..f4a329f 100644
> --- a/data/src/pmxcfs.c
> +++ b/data/src/pmxcfs.c
> @@ -61,6 +61,7 @@
>  #define RESTART_FLAG_FILE RUNDIR "/cfs-restart-flag"
>  
>  #define CFSDIR "/etc/pve"
> +#define OK_MESSAGE "OK"

s/OK_MESSAGE/READY_MESSAGE/ ?
or SYNC_MESSAGE, although that is potentially confusing in the context
of pmcfs ;)

a single char 'S' would also be sufficient as message I think (for
'startup' or 'sync')

>  
>  cfs_t cfs = {
>  	.debug = 0,
> @@ -775,6 +776,9 @@ int main(int argc, char *argv[])
>  {
>  	int ret = -1;
>  	int lockfd = -1;
> +	int pipefd[2];
> +	int readbytes;
> +	char readbuffer[strlen(OK_MESSAGE)];
>  
>  	gboolean foreground = FALSE;
>  	gboolean force_local_mode = FALSE;
> @@ -954,17 +958,31 @@ int main(int argc, char *argv[])
>  	fuse_set_signal_handlers(fuse_get_session(fuse));
>  
>  	if (!foreground) {
> +		pipe(pipefd);
>  		pid_t cpid = fork();
>  
>  		if (cpid == -1) {
>  			cfs_critical("failed to daemonize program - %s", strerror (errno));
>  			goto err;
>  		} else if (cpid) {
> -			write_pidfile(cpid);
> -			qb_log_fini();
> -			_exit (0);
> +			close(pipefd[1]);
> +			readbytes = read(pipefd[0], readbuffer, sizeof(readbuffer));
> +			close(pipefd[0]);
> +			if (readbytes == 2 &&

hardcoded 2 bytes here vs. sizeof/strlen everywhere else..

> +			    strncmp(readbuffer, OK_MESSAGE, strlen(OK_MESSAGE)) == 0) {
> +			    /* child finished starting up */
> +			    write_pidfile(cpid);
> +			    qb_log_fini();
> +			    _exit (0);
> +			} else {
> +			    /* something went wrong */
> +			    cfs_critical("child failed to send OK");
> +			    goto err;
> +			}
> +
>  		} else {
>  			int nullfd;
> +			close(pipefd[0]);
>  
>  			chroot("/");
>  
> @@ -1022,6 +1040,10 @@ int main(int argc, char *argv[])
>  
>  	unlink(RESTART_FLAG_FILE);
>  
> +	/* finished starting up, signaling parent */
> +	write(pipefd[1], OK_MESSAGE, strlen(OK_MESSAGE));
> +	close(pipefd[1]);
> +
>  	ret = fuse_loop_mt(fuse);
>  
>  	open(RESTART_FLAG_FILE, O_CREAT|O_NOCTTY|O_NONBLOCK);
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel