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

[Wolfgang Bumiller]

On Thu, Apr 12, 2018 at 12:37:16PM +0200, Dominik Csapak wrote:
> since systemd depends that parent exits 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
> 
> without this, we had an issue, where the
> ExecStartPost hook (which runs pvecm updatecerts) did not run reliably,
> but which is necessary to setup the nodes/ dir in /etc/pve
> and generating the ssl certificates
> 
> this could also affect every service which has an
> After=pve-cluster
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v1:
> * only use one byte for message ('1')
> * check error of read/pipe
> * wrap write in 'if (!foreground)' to not write with pmxcfs -l
>  data/src/pmxcfs.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/data/src/pmxcfs.c b/data/src/pmxcfs.c
> index 6047ad0..f6a2749 100644
> --- a/data/src/pmxcfs.c
> +++ b/data/src/pmxcfs.c
> @@ -775,6 +775,9 @@ int main(int argc, char *argv[])
>  {
>  	int ret = -1;
>  	int lockfd = -1;
> +	int pipefd[2];
> +	int readbytes;
> +	char readbuffer;

Since these are only used below and a v3 is necessary (see below),
it would make sense to declare readbytes and readbuffer below.

>  
>  	gboolean foreground = FALSE;
>  	gboolean force_local_mode = FALSE;
> @@ -954,17 +957,34 @@ int main(int argc, char *argv[])
>  	fuse_set_signal_handlers(fuse_get_session(fuse));
>  
>  	if (!foreground) {
> +		if (pipe(pipefd) == -1) {
> +			cfs_critical("pipe error: %s", strerror(errno));
> +			goto err;
> +		}
> +
>  		pid_t cpid = fork();
>  
>  		if (cpid == -1) {
>  			cfs_critical("failed to daemonize program - %s", strerror (errno));
>  			goto err;
>  		} else if (cpid) {
> +			close(pipefd[1]);
> +			readbytes = read(pipefd[0], &readbuffer, sizeof(readbuffer));
> +			close(pipefd[0]);

This call overrides errno, either save it before close() or move the
close() call down.

> +			if (readbytes == -1) {
> +				cfs_critical("read error: %s", strerror(errno));

Both error cases should probably kill(cpid, SIGKILL) to be sure.

> +				goto err;
> +			} else if (readbytes != 1 || readbuffer != '1') {
> +				cfs_critical("child failed to send '1'");
> +				goto err;
> +			}
> +			/* child finished starting up */
>  			write_pidfile(cpid);
>  			qb_log_fini();
>  			_exit (0);
>  		} else {
>  			int nullfd;
> +			close(pipefd[0]);
>  
>  			chroot("/");
>  
> @@ -1022,6 +1042,12 @@ int main(int argc, char *argv[])
>  
>  	unlink(RESTART_FLAG_FILE);
>  
> +	if (!foreground) {
> +		/* finished starting up, signaling parent */
> +		write(pipefd[1], "1", 1);
> +		close(pipefd[1]);
> +	}
> +
>  	ret = fuse_loop_mt(fuse);
>  
>  	open(RESTART_FLAG_FILE, O_CREAT|O_NOCTTY|O_NONBLOCK);
> -- 
> 2.11.0