diff mbox series

[2/3] qsd: Add --daemonize

Message ID 20211222114153.67721-3-hreitz@redhat.com
State New
Headers show
Series qsd: Add --daemonize; and add job quit tests | expand

Commit Message

Hanna Czenczek Dec. 22, 2021, 11:41 a.m. UTC
This option does basically the same as --fork does for qemu-nbd:
- We fork off a child process
- The child process is daemonized (closing its stdin and stdout)
- stderr of the child is routed through the parent, so the parent can
  see errors and adjust its exit code accordingly
- Once the child closes its end of this stderr pipe (done right after
  creating the PID file), the parent exits

It is not named --fork, because --fork was probably a name that few
programs but qemu-nbd ever used.  qemu (the system emulator) itself uses
-daemonize, too.  (Besides, QSD's interface is not compatible to
qemu-nbd anyway; compare --pidfile vs. --pid-file.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 docs/tools/qemu-storage-daemon.rst   |   7 ++
 storage-daemon/qemu-storage-daemon.c | 151 +++++++++++++++++++++++++++
 2 files changed, 158 insertions(+)

Comments

Vladimir Sementsov-Ogievskiy Dec. 30, 2021, 4:12 p.m. UTC | #1
22.12.2021 14:41, Hanna Reitz wrote:
> This option does basically the same as --fork does for qemu-nbd:

Can we share the code?

Before this patch we already have --fork code-path of qemu-nbd and -daemonize code-path of QEMU.. Now we have one more. Did you consider improving and sharing the old code instead?

> - We fork off a child process
> - The child process is daemonized (closing its stdin and stdout)
> - stderr of the child is routed through the parent, so the parent can
>    see errors and adjust its exit code accordingly
> - Once the child closes its end of this stderr pipe (done right after
>    creating the PID file), the parent exits
> 
> It is not named --fork, because --fork was probably a name that few
> programs but qemu-nbd ever used.  qemu (the system emulator) itself uses
> -daemonize, too.  (Besides, QSD's interface is not compatible to
> qemu-nbd anyway; compare --pidfile vs. --pid-file.)
> 
> Signed-off-by: Hanna Reitz<hreitz@redhat.com>
Hanna Czenczek Jan. 3, 2022, 5:15 p.m. UTC | #2
On 30.12.21 17:12, Vladimir Sementsov-Ogievskiy wrote:
> 22.12.2021 14:41, Hanna Reitz wrote:
>> This option does basically the same as --fork does for qemu-nbd:
>
> Can we share the code?
>
> Before this patch we already have --fork code-path of qemu-nbd and 
> -daemonize code-path of QEMU.. Now we have one more. Did you consider 
> improving and sharing the old code instead?

I didn’t really, to be honest.  I disliked sharing code with qemu-nbd, 
because, well, that would mean touching qemu-nbd (which I’d rather 
avoid).  Then again, I suppose we could theoretically just drop the 
daemonizing code from qemu-nbd, and replace it by calls to daemonize() 
and daemon_detach().  The only problem with that would be that we need 
some file into which to put it, that is linked into both qemu-nbd and 
the QSD.

QEMU proper already has os_daemonize() and os_setup_post(), but they’re 
quite different from what qemu-nbd does, for example, it doesn’t call 
qemu_daemon(), and it chdir()s to / (which we probably don’t want?).

I preferred to go with what qemu-nbd does, because I thought if it works 
for qemu-nbd, it should work for QSD, too.  Maybe I’m wrong, though, 
maybe we should just use os_daemonize() and os_setup_post().

Hanna
diff mbox series

Patch

diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
index 3e5a9dc032..83905ad526 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -149,6 +149,13 @@  Standard options:
   created but before accepting connections. The daemon has started successfully
   when the pid file is written and clients may begin connecting.
 
+.. option:: --daemonize
+
+  Daemonize the process. The parent process will exit once startup is complete
+  (i.e., after the pid file has been or would have been written) or failure
+  occurs. Its exit code reflects whether the child has started up successfully
+  or failed to do so.
+
 Examples
 --------
 Launch the daemon with QMP monitor socket ``qmp.sock`` so clients can execute
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 42a52d3b1c..cc94240545 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -60,6 +60,7 @@ 
 #include "trace/control.h"
 
 static const char *pid_file;
+static bool daemonize_opt;
 static volatile bool exit_requested = false;
 
 void qemu_system_killed(int signal, pid_t pid)
@@ -124,6 +125,9 @@  static void help(void)
 "\n"
 "  --pidfile <path>       write process ID to a file after startup\n"
 "\n"
+"  --daemonize            daemonize the process, and have the parent exit\n"
+"                         once startup is complete\n"
+"\n"
 QEMU_HELP_BOTTOM "\n",
     error_get_progname());
 }
@@ -131,6 +135,7 @@  QEMU_HELP_BOTTOM "\n",
 enum {
     OPTION_BLOCKDEV = 256,
     OPTION_CHARDEV,
+    OPTION_DAEMONIZE,
     OPTION_EXPORT,
     OPTION_MONITOR,
     OPTION_NBD_SERVER,
@@ -187,6 +192,7 @@  static void process_options(int argc, char *argv[], bool pre_init_pass)
     static const struct option long_options[] = {
         {"blockdev", required_argument, NULL, OPTION_BLOCKDEV},
         {"chardev", required_argument, NULL, OPTION_CHARDEV},
+        {"daemonize", no_argument, NULL, OPTION_DAEMONIZE},
         {"export", required_argument, NULL, OPTION_EXPORT},
         {"help", no_argument, NULL, 'h'},
         {"monitor", required_argument, NULL, OPTION_MONITOR},
@@ -212,6 +218,7 @@  static void process_options(int argc, char *argv[], bool pre_init_pass)
             c == '?' ||
             c == 'h' ||
             c == 'V' ||
+            c == OPTION_DAEMONIZE ||
             c == OPTION_PIDFILE;
 
         /* Process every option only in its respective pass */
@@ -264,6 +271,9 @@  static void process_options(int argc, char *argv[], bool pre_init_pass)
                 qemu_opts_del(opts);
                 break;
             }
+        case OPTION_DAEMONIZE:
+            daemonize_opt = true;
+            break;
         case OPTION_EXPORT:
             {
                 Visitor *v;
@@ -342,8 +352,137 @@  static void pid_file_init(void)
     atexit(pid_file_cleanup);
 }
 
+/**
+ * Handle daemonizing.
+ *
+ * Return false on error, and true if and only if daemonizing was
+ * successful and we are in the child process.  (The parent process will
+ * never return true, but instead rather exit() if there was no error.)
+ *
+ * When returning true, *old_stderr is set to an FD representing the
+ * original stderr.  Once the child is set up (after creating the PID
+ * file, and before entering the main loop), it should invoke
+ * `daemon_detach(old_stderr)` to have the parent process exit and
+ * restore the original stderr.
+ */
+static bool daemonize(int *old_stderr, Error **errp)
+{
+    int stderr_fd[2];
+    pid_t pid;
+    int ret;
+
+    if (qemu_pipe(stderr_fd) < 0) {
+        error_setg_errno(errp, errno, "Error setting up communication pipe");
+        return false;
+    }
+
+    pid = fork();
+    if (pid < 0) {
+        error_setg_errno(errp, errno, "Failed to fork");
+        return false;
+    }
+
+    if (pid == 0) { /* Child process */
+        close(stderr_fd[0]); /* Close pipe's read end */
+
+        /* Keep the old stderr so we can reuse it after the parent has quit */
+        *old_stderr = dup(STDERR_FILENO);
+        if (*old_stderr < 0) {
+            /*
+             * Cannot return an error without having our stderr point to the
+             * pipe: Otherwise, the parent process would not see the error
+             * message and so not exit with EXIT_FAILURE.
+             */
+            error_setg_errno(errp, errno, "Failed to duplicate stderr FD");
+            dup2(stderr_fd[1], STDERR_FILENO);
+            close(stderr_fd[1]);
+            return false;
+        }
+
+        /*
+         * Daemonize, redirecting all std streams to /dev/null; then
+         * (even on error) redirect stderr to the pipe's write end
+         */
+        ret = qemu_daemon(1, 0);
+
+        /*
+         * Unconditionally redirect stderr to the pipe's write end (and
+         * close the then-unused write end FD, because now stderr points
+         * to it)
+         */
+        dup2(stderr_fd[1], STDERR_FILENO);
+        close(stderr_fd[1]);
+
+        if (ret < 0) {
+            error_setg_errno(errp, errno, "Failed to daemonize");
+            close(*old_stderr);
+            *old_stderr = -1;
+            return false;
+        }
+
+        return true;
+    } else { /* Parent process */
+        bool errors = false;
+        g_autofree char *buf = g_malloc(1024);
+
+        close(stderr_fd[1]); /* Close pipe's write end */
+
+        /* Print error messages from the child until it closes the pipe */
+        while ((ret = read(stderr_fd[0], buf, 1024)) > 0) {
+            errors = true;
+            ret = qemu_write_full(STDERR_FILENO, buf, ret);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret,
+                                 "Failed to print error received from the "
+                                 "daemonized child to stderr");
+                close(stderr_fd[0]);
+                return false;
+            }
+        }
+
+        close(stderr_fd[0]); /* Close read end, it is unused now */
+
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Cannot read from daemon");
+            return false;
+        }
+
+        /*
+         * Child is either detached and running (in which case it should
+         * not have printed any errors, and @errors should be false), or
+         * has encountered an error (which it should have printed, so
+         * @errors should be true) and has exited.
+         *
+         * Exit with the appropriate exit code.
+         */
+        exit(errors ? EXIT_FAILURE : EXIT_SUCCESS);
+    }
+}
+
+/**
+ * After daemonize(): Let the parent process exit by closing the pipe
+ * connected to it.  The original stderr is restored from *old_stderr.
+ *
+ * This function should be called after creating the PID file and before
+ * entering the main loop.
+ */
+static void daemon_detach(int *old_stderr)
+{
+    /*
+     * Ignore errors; closing the old stderr should not fail, and if
+     * dup-ing fails, then we cannot print anything to stderr anyway
+     */
+    dup2(*old_stderr, STDERR_FILENO);
+
+    close(*old_stderr);
+    *old_stderr = -1;
+}
+
 int main(int argc, char *argv[])
 {
+    Error *local_err = NULL;
+    int old_stderr = -1;
+
 #ifdef CONFIG_POSIX
     signal(SIGPIPE, SIG_IGN);
 #endif
@@ -354,6 +493,14 @@  int main(int argc, char *argv[])
 
     process_options(argc, argv, true);
 
+    if (daemonize_opt) {
+        if (!daemonize(&old_stderr, &local_err)) {
+            error_report_err(local_err);
+            return EXIT_FAILURE;
+        }
+        assert(old_stderr >= 0);
+    }
+
     module_call_init(MODULE_INIT_QOM);
     module_call_init(MODULE_INIT_TRACE);
     qemu_add_opts(&qemu_trace_opts);
@@ -377,6 +524,10 @@  int main(int argc, char *argv[])
      */
     pid_file_init();
 
+    if (daemonize_opt) {
+        daemon_detach(&old_stderr);
+    }
+
     while (!exit_requested) {
         main_loop_wait(false);
     }