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

2018-04-12 Thread 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 
> ---
> 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

___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


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

2018-04-12 Thread Dominik Csapak
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 
---
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;
 
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]);
+   if (readbytes == -1) {
+   cfs_critical("read error: %s", strerror(errno));
+   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


___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel