Re: [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 
> ---
>  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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

Re: [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 
> ---
>  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

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

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

[Dominik Csapak]

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 
---
 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"
 
 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 &&
+   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@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel