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.csa...@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
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
