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

[Wolfgang Bumiller]

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
> 
> 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
> 
> 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"

It's a simple enough case to just use a single letter '1' (or the actual
value 1) instead ;-)

>  
>  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)];

Then this can just be a char instead of an array (otherwise it's nicer
to use `sizeof(X)-1` instead of strlen() as on lower optimization levels
this is technically a VLA to the compiler when using strlen().

>  
>  	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);

You need to check the return value here.

>  		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);

Totally optional: it's probably nicer to have just the error case in an
if() and have this code at the end of this block. Looks more final ;-)

> +			close(pipefd[1]);
> +			readbytes = read(pipefd[0], readbuffer, sizeof(readbuffer));
> +			close(pipefd[0]);
> +			if (readbytes == 2 &&
> +			    strncmp(readbuffer, OK_MESSAGE, strlen(OK_MESSAGE)) == 0) {

And when just sending one byte you don't need the long strncmp and the
line is much more concise ;-)

> +			    /* 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]);

This needs to also be guarded by the `foreground` variable otherwise
`pmxcfs -l` will try to write to an uninitialized handle here, and/or
worse, write to a random valid one and close it ;-)

> +
>  	ret = fuse_loop_mt(fuse);
>  
>  	open(RESTART_FLAG_FILE, O_CREAT|O_NOCTTY|O_NONBLOCK);
> -- 
> 2.11.0