[PATCH 0/3] qsd: Add --daemonize; and add job quit tests

[PATCH 0/3] qsd: Add --daemonize; and add job quit tests

[Hanna Reitz]

Hi,

This series began as an attempt to write a reproducer for the following
case:

  You have a mirror job in READY state with a target that’s slow.  There
  is still a lot of data to be written (because of active I/O in the
  guest).  You quit qemu, and you expect quitting to be reasonably
  quick.
  Old qemu used to still complete the job, which may take forever, new
  qemu cancels the job, which is better.

That’s basically patch 3 in this series, which tests this behavior once
for mirror and once for active commit.

Problem is, how to simulate a slow target for this; if you use a
throttle node in the same qemu process that you’re trying to test, qemu
will just drain it when quitting, and so the supposedly slow target
becomes very fast.

So we need an external instance, and what better to use but the storage
daemon.  I found that it would be nice if for this it had a --daemonize
option (well, it would be nice in general), and so the first two patches
implement that.


Hanna Reitz (3):
  qsd: Add pre-init argument parsing pass
  qsd: Add --daemonize
  iotests/185: Add post-READY quit tests

 docs/tools/qemu-storage-daemon.rst   |   7 +
 storage-daemon/qemu-storage-daemon.c | 188 +++++++++++++++++++++++++-
 tests/qemu-iotests/185               | 190 ++++++++++++++++++++++++++-
 tests/qemu-iotests/185.out           |  48 +++++++
 4 files changed, 430 insertions(+), 3 deletions(-)

-- 
2.33.1

[PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

We want to add a --daemonize argument to QSD's command line.  This will
require forking the process before we do any complex initialization
steps, like setting up the block layer or QMP.  Therefore, we must scan
the command line for it long before our current process_options() call.

Instead of adding custom new code to do so, just reuse process_options()
and give it a @pre_init_pass argument to distinguish the two passes.  I
believe there are some other switches but --daemonize that deserve
parsing in the first pass:

- --help and --version are supposed to only print some text and then
  immediately exit (so any initialization we do would be for naught).
  This changes behavior, because now "--blockdev inv-drv --help" will
  print a help text instead of complaining about the --blockdev
  argument.
  Note that this is similar in behavior to other tools, though: "--help"
  is generally immediately acted upon when finding it in the argument
  list, potentially before other arguments (even ones before it) are
  acted on.  For example, "ls /does-not-exist --help" prints a help text
  and does not complain about ENOENT.

- --pidfile does not need initialization, and is already exempted from
  the sequential order that process_options() claims to strictly follow
  (the PID file is only created after all arguments are processed, not
  at the time the --pidfile argument appears), so it makes sense to
  include it in the same category as --daemonize.

- Invalid arguments should always be reported as soon as possible.  (The
  same caveat with --help applies: That means that "--blockdev inv-drv
  --inv-arg" will now complain about --inv-arg, not inv-drv.)

Note that we could decide to check only for --daemonize in the first
pass, and defer --help, --version, and checking for invalid arguments to
the second one, thus largely keeping our current behavior.  However,
this would break "--help --daemonize": The child would print the help
text to stdout, which is redirected to /dev/null, and so the text would
disappear.  We would need to have the text be printed to stderr instead,
and this would then make the parent process exit with EXIT_FAILURE,
which is probably not what we want for --help.

This patch does make some references to --daemonize without having
implemented it yet, but that will happen in the next patch.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c | 37 ++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 52cf17e8ac..42a52d3b1c 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -164,7 +164,23 @@ static int getopt_set_loc(int argc, char **argv, const char *optstring,
     return c;
 }
 
-static void process_options(int argc, char *argv[])
+/**
+ * Process QSD command-line arguments.
+ *
+ * This is done in two passes:
+ *
+ * First (@pre_init_pass is true), we do a pass where all global
+ * arguments pertaining to the QSD process (like --help or --daemonize)
+ * are processed.  This pass is done before most of the QEMU-specific
+ * initialization steps (e.g. initializing the block layer or QMP), and
+ * so must only process arguments that are not really QEMU-specific.
+ *
+ * Second (@pre_init_pass is false), we (sequentially) process all
+ * QEMU/QSD-specific arguments.  Many of these arguments are effectively
+ * translated to QMP commands (like --blockdev for blockdev-add, or
+ * --export for block-export-add).
+ */
+static void process_options(int argc, char *argv[], bool pre_init_pass)
 {
     int c;
 
@@ -187,7 +203,22 @@ static void process_options(int argc, char *argv[])
      * they are given on the command lines. This means that things must be
      * defined first before they can be referenced in another option.
      */
+    optind = 1;
     while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) != -1) {
+        bool handle_option_pre_init;
+
+        /* Should this argument be processed in the pre-init pass? */
+        handle_option_pre_init =
+            c == '?' ||
+            c == 'h' ||
+            c == 'V' ||
+            c == OPTION_PIDFILE;
+
+        /* Process every option only in its respective pass */
+        if (pre_init_pass != handle_option_pre_init) {
+            continue;
+        }
+
         switch (c) {
         case '?':
             exit(EXIT_FAILURE);
@@ -321,6 +352,8 @@ int main(int argc, char *argv[])
     qemu_init_exec_dir(argv[0]);
     os_setup_signal_handling();
 
+    process_options(argc, argv, true);
+
     module_call_init(MODULE_INIT_QOM);
     module_call_init(MODULE_INIT_TRACE);
     qemu_add_opts(&qemu_trace_opts);
@@ -335,7 +368,7 @@ int main(int argc, char *argv[])
     qemu_set_log(LOG_TRACE);
 
     qemu_init_main_loop(&error_fatal);
-    process_options(argc, argv);
+    process_options(argc, argv, false);
 
     /*
      * Write the pid file after creating chardevs, exports, and NBD servers but
-- 
2.33.1

[PATCH 2/3] qsd: Add --daemonize

[Hanna Reitz]

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(+)

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

[PATCH 3/3] iotests/185: Add post-READY quit tests

[Hanna Reitz]

185 tests quitting qemu while a block job is active.  It does not
specifically test quitting qemu while a mirror or active commit job is
in its READY phase.

Add two test cases for this, where we respectively mirror or commit to
an external QSD instance, which provides a throttled block device.  qemu
is supposed to cancel the job so that it can quit as soon as possible
instead of waiting for the job to complete (which it did before 6.2).

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/185     | 190 ++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/185.out |  48 ++++++++++
 2 files changed, 237 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
index f2ec5c5ceb..8b1143dc16 100755
--- a/tests/qemu-iotests/185
+++ b/tests/qemu-iotests/185
@@ -33,6 +33,12 @@ _cleanup()
     _rm_test_img "${TEST_IMG}.copy"
     _cleanup_test_img
     _cleanup_qemu
+
+    if [ -f "$TEST_DIR/qsd.pid" ]; then
+        kill -SIGKILL "$(cat "$TEST_DIR/qsd.pid")"
+        rm -f "$TEST_DIR/qsd.pid"
+    fi
+    rm -f "$SOCK_DIR/qsd.sock"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -45,7 +51,7 @@ _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 
-size=64M
+size=$((64 * 1048576))
 TEST_IMG="${TEST_IMG}.base" _make_test_img $size
 
 echo
@@ -216,6 +222,188 @@ wait=1 _cleanup_qemu | grep -v 'JOB_STATUS_CHANGE'
 
 _check_test_img
 
+echo
+echo === Start mirror to throttled QSD and exit qemu ===
+echo
+
+# Mirror to a throttled QSD instance (so that qemu cannot drain the
+# throttling), wait for READY, then write some data to the device,
+# and then quit qemu.
+# (qemu should force-cancel the job and not wait for the data to be
+# written to the target.)
+
+_make_test_img $size
+
+# Will be used by this and the next case
+set_up_throttled_qsd() {
+    $QSD \
+        --object throttle-group,id=thrgr,limits.bps-total=1048576 \
+        --blockdev null-co,node-name=null,size=$size \
+        --blockdev throttle,node-name=throttled,throttle-group=thrgr,file=null \
+        --nbd-server addr.type=unix,addr.path="$SOCK_DIR/qsd.sock" \
+        --export nbd,id=exp,node-name=throttled,name=target,writable=true \
+        --pidfile "$TEST_DIR/qsd.pid" \
+        --daemonize
+}
+
+set_up_throttled_qsd
+
+# Need a virtio-blk device so that qemu-io writes will not block the monitor
+_launch_qemu \
+    --blockdev file,node-name=source-proto,filename="$TEST_IMG" \
+    --blockdev qcow2,node-name=source-fmt,file=source-proto \
+    --device virtio-blk,id=vblk,drive=source-fmt \
+    --blockdev "{\"driver\": \"nbd\",
+                 \"node-name\": \"target\",
+                 \"server\": {
+                     \"type\": \"unix\",
+                     \"path\": \"$SOCK_DIR/qsd.sock\"
+                 },
+                 \"export\": \"target\"}"
+
+h=$QEMU_HANDLE
+_send_qemu_cmd $h '{"execute": "qmp_capabilities"}' 'return'
+
+# Use sync=top, so the first pass will not copy the whole image
+_send_qemu_cmd $h \
+    '{"execute": "blockdev-mirror",
+      "arguments": {
+          "job-id": "mirror",
+          "device": "source-fmt",
+          "target": "target",
+          "sync": "top"
+      }}' \
+    'return' \
+    | grep -v JOB_STATUS_CHANGE # Ignore these events during creation
+
+# This too will be used by this and the next case
+# $1: QEMU handle
+# $2: Image size
+wait_for_job_and_quit() {
+    h=$1
+    size=$2
+
+    # List of expected events
+    capture_events='BLOCK_JOB_READY JOB_STATUS_CHANGE'
+    _wait_event $h 'BLOCK_JOB_READY'
+    QEMU_EVENTS= # Ignore all JOB_STATUS_CHANGE events that came before READY
+
+    # Write something to the device for post-READY mirroring.  Write it in
+    # blocks matching the cluster size, each spaced one block apart, so
+    # that the mirror job will have to spawn one request per cluster.
+    # Because the number of concurrent requests is limited (to 16), this
+    # limits the number of bytes concurrently in flight, which speeds up
+    # cancelling the job (in-flight requests still are waited for).
+    # To limit the number of bytes in flight, we could alternatively pass
+    # something for blockdev-mirror's @buf-size parameter, but
+    # block-commit does not have such a parameter, so we need to figure
+    # something out that works for both.
+
+    cluster_size=65536
+    step=$((cluster_size * 2))
+
+    echo '--- Writing data to the virtio-blk device ---'
+
+    for ofs in $(seq 0 $step $((size - step))); do
+        qemu_io_cmd="qemu-io -d vblk/virtio-backend "
+        qemu_io_cmd+="\\\"aio_write $ofs $cluster_size\\\""
+
+        # Do not include these requests in the reference output
+        # (it's just too much)
+        silent=yes _send_qemu_cmd $h \
+            "{\"execute\": \"human-monitor-command\",
+              \"arguments\": {
+                  \"command-line\": \"$qemu_io_cmd\"
+              }}" \
+            'return'
+    done
+
+    # Wait until the job's length is updated to reflect the write requests
+
+    # We have written to half of the device, so this is the expected job length
+    final_len=$((size / 2))
+    timeout=100 # unit: 0.1 seconds
+    while true; do
+        len=$(
+            _send_qemu_cmd $h \
+                '{"execute": "query-block-jobs"}' \
+                'return.*"len": [0-9]\+' \
+                | grep 'return.*"len": [0-9]\+' \
+                | sed -e 's/.*"len": \([0-9]\+\).*/\1/'
+        )
+        if [ "$len" -eq "$final_len" ]; then
+            break
+        fi
+        timeout=$((timeout - 1))
+        if [ "$timeout" -eq 0 ]; then
+            echo "ERROR: Timeout waiting for job to reach len=$final_len"
+            break
+        fi
+        sleep 0.1
+    done
+
+    sleep 1
+
+    _send_qemu_cmd $h \
+        '{"execute": "quit"}' \
+        'return'
+
+    # List of expected events
+    capture_events='BLOCK_JOB_CANCELLED JOB_STATUS_CHANGE SHUTDOWN'
+    _wait_event $h 'SHUTDOWN'
+    QEMU_EVENTS= # Ignore all JOB_STATUS_CHANGE events that came before SHUTDOWN
+    _wait_event $h 'JOB_STATUS_CHANGE' # standby
+    _wait_event $h 'JOB_STATUS_CHANGE' # ready
+    _wait_event $h 'JOB_STATUS_CHANGE' # aborting
+    # Filter the offset (depends on when exactly `quit` was issued)
+    _wait_event $h 'BLOCK_JOB_CANCELLED' \
+        | sed -e 's/"offset": [0-9]\+/"offset": (filtered)/'
+    _wait_event $h 'JOB_STATUS_CHANGE' # concluded
+    _wait_event $h 'JOB_STATUS_CHANGE' # null
+
+    wait=yes _cleanup_qemu
+
+    kill -SIGTERM "$(cat "$TEST_DIR/qsd.pid")"
+}
+
+wait_for_job_and_quit $h $size
+
+echo
+echo === Start active commit to throttled QSD and exit qemu ===
+echo
+
+# Same as the above, but instead of mirroring, do an active commit
+
+_make_test_img $size
+
+set_up_throttled_qsd
+
+_launch_qemu \
+    --blockdev "{\"driver\": \"nbd\",
+                 \"node-name\": \"target\",
+                 \"server\": {
+                     \"type\": \"unix\",
+                     \"path\": \"$SOCK_DIR/qsd.sock\"
+                 },
+                 \"export\": \"target\"}" \
+    --blockdev file,node-name=source-proto,filename="$TEST_IMG" \
+    --blockdev qcow2,node-name=source-fmt,file=source-proto,backing=target \
+    --device virtio-blk,id=vblk,drive=source-fmt
+
+h=$QEMU_HANDLE
+_send_qemu_cmd $h '{"execute": "qmp_capabilities"}' 'return'
+
+_send_qemu_cmd $h \
+    '{"execute": "block-commit",
+      "arguments": {
+          "job-id": "commit",
+          "device": "source-fmt"
+      }}' \
+    'return' \
+    | grep -v JOB_STATUS_CHANGE # Ignore these events during creation
+
+wait_for_job_and_quit $h $size
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
index 754a641258..70e8dd6c87 100644
--- a/tests/qemu-iotests/185.out
+++ b/tests/qemu-iotests/185.out
@@ -116,4 +116,52 @@ Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 cluster_size=65536 extended_l2=off
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "stream"}}
 No errors were found on the image.
+
+=== Start mirror to throttled QSD and exit qemu ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "blockdev-mirror",
+      "arguments": {
+          "job-id": "mirror",
+          "device": "source-fmt",
+          "target": "target",
+          "sync": "top"
+      }}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "mirror", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
+--- Writing data to the virtio-blk device ---
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "mirror", "len": 33554432, "offset": (filtered), "speed": 0, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "mirror"}}
+
+=== Start active commit to throttled QSD and exit qemu ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "block-commit",
+      "arguments": {
+          "job-id": "commit",
+          "device": "source-fmt"
+      }}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "commit", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+--- Writing data to the virtio-blk device ---
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "commit", "len": 33554432, "offset": (filtered), "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "commit"}}
 *** done
-- 
2.33.1

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Vladimir Sementsov-Ogievskiy]

22.12.2021 14:41, Hanna Reitz wrote:
> We want to add a --daemonize argument to QSD's command line.  This will
> require forking the process before we do any complex initialization
> steps, like setting up the block layer or QMP.  Therefore, we must scan
> the command line for it long before our current process_options() call.
> 
> Instead of adding custom new code to do so, just reuse process_options()
> and give it a @pre_init_pass argument to distinguish the two passes.  I
> believe there are some other switches but --daemonize that deserve
> parsing in the first pass:
> 
> - --help and --version are supposed to only print some text and then
>    immediately exit (so any initialization we do would be for naught).
>    This changes behavior, because now "--blockdev inv-drv --help" will
>    print a help text instead of complaining about the --blockdev
>    argument.
>    Note that this is similar in behavior to other tools, though: "--help"
>    is generally immediately acted upon when finding it in the argument
>    list, potentially before other arguments (even ones before it) are
>    acted on.  For example, "ls /does-not-exist --help" prints a help text
>    and does not complain about ENOENT.
> 
> - --pidfile does not need initialization, and is already exempted from
>    the sequential order that process_options() claims to strictly follow
>    (the PID file is only created after all arguments are processed, not
>    at the time the --pidfile argument appears), so it makes sense to
>    include it in the same category as --daemonize.
> 
> - Invalid arguments should always be reported as soon as possible.  (The
>    same caveat with --help applies: That means that "--blockdev inv-drv
>    --inv-arg" will now complain about --inv-arg, not inv-drv.)
> 
> Note that we could decide to check only for --daemonize in the first
> pass, and defer --help, --version, and checking for invalid arguments to
> the second one, thus largely keeping our current behavior.  However,
> this would break "--help --daemonize": The child would print the help
> text to stdout, which is redirected to /dev/null, and so the text would
> disappear.  We would need to have the text be printed to stderr instead,
> and this would then make the parent process exit with EXIT_FAILURE,
> which is probably not what we want for --help.
> 
> This patch does make some references to --daemonize without having
> implemented it yet, but that will happen in the next patch.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   storage-daemon/qemu-storage-daemon.c | 37 ++++++++++++++++++++++++++--
>   1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
> index 52cf17e8ac..42a52d3b1c 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -164,7 +164,23 @@ static int getopt_set_loc(int argc, char **argv, const char *optstring,
>       return c;
>   }
>   
> -static void process_options(int argc, char *argv[])
> +/**
> + * Process QSD command-line arguments.
> + *
> + * This is done in two passes:
> + *
> + * First (@pre_init_pass is true), we do a pass where all global
> + * arguments pertaining to the QSD process (like --help or --daemonize)
> + * are processed.  This pass is done before most of the QEMU-specific
> + * initialization steps (e.g. initializing the block layer or QMP), and
> + * so must only process arguments that are not really QEMU-specific.
> + *
> + * Second (@pre_init_pass is false), we (sequentially) process all
> + * QEMU/QSD-specific arguments.  Many of these arguments are effectively
> + * translated to QMP commands (like --blockdev for blockdev-add, or
> + * --export for block-export-add).
> + */
> +static void process_options(int argc, char *argv[], bool pre_init_pass)
>   {
>       int c;
>   
> @@ -187,7 +203,22 @@ static void process_options(int argc, char *argv[])
>        * they are given on the command lines. This means that things must be

So, --pidfile already breaks a bit this comment. Still would be good to adjust it now..

may be, s/options/QEMU-specific options/ or something like this.

>        * defined first before they can be referenced in another option.
>        */
> +    optind = 1;
>       while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) != -1) {
> +        bool handle_option_pre_init;
> +
> +        /* Should this argument be processed in the pre-init pass? */
> +        handle_option_pre_init =
> +            c == '?' ||
> +            c == 'h' ||
> +            c == 'V' ||
> +            c == OPTION_PIDFILE;
> +
> +        /* Process every option only in its respective pass */
> +        if (pre_init_pass != handle_option_pre_init) {
> +            continue;
> +        }
> +
>           switch (c) {
>           case '?':
>               exit(EXIT_FAILURE);
> @@ -321,6 +352,8 @@ int main(int argc, char *argv[])
>       qemu_init_exec_dir(argv[0]);
>       os_setup_signal_handling();
>   
> +    process_options(argc, argv, true);
> +
>       module_call_init(MODULE_INIT_QOM);
>       module_call_init(MODULE_INIT_TRACE);
>       qemu_add_opts(&qemu_trace_opts);
> @@ -335,7 +368,7 @@ int main(int argc, char *argv[])
>       qemu_set_log(LOG_TRACE);
>   
>       qemu_init_main_loop(&error_fatal);
> -    process_options(argc, argv);
> +    process_options(argc, argv, false);
>   
>       /*
>        * Write the pid file after creating chardevs, exports, and NBD servers but
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/3] qsd: Add --daemonize

[Vladimir Sementsov-Ogievskiy]

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>


-- 
Best regards,
Vladimir

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

On 30.12.21 17:00, Vladimir Sementsov-Ogievskiy wrote:
> 22.12.2021 14:41, Hanna Reitz wrote:
>> We want to add a --daemonize argument to QSD's command line.  This will
>> require forking the process before we do any complex initialization
>> steps, like setting up the block layer or QMP.  Therefore, we must scan
>> the command line for it long before our current process_options() call.
>>
>> Instead of adding custom new code to do so, just reuse process_options()
>> and give it a @pre_init_pass argument to distinguish the two passes.  I
>> believe there are some other switches but --daemonize that deserve
>> parsing in the first pass:
>>
>> - --help and --version are supposed to only print some text and then
>>    immediately exit (so any initialization we do would be for naught).
>>    This changes behavior, because now "--blockdev inv-drv --help" will
>>    print a help text instead of complaining about the --blockdev
>>    argument.
>>    Note that this is similar in behavior to other tools, though: 
>> "--help"
>>    is generally immediately acted upon when finding it in the argument
>>    list, potentially before other arguments (even ones before it) are
>>    acted on.  For example, "ls /does-not-exist --help" prints a help 
>> text
>>    and does not complain about ENOENT.
>>
>> - --pidfile does not need initialization, and is already exempted from
>>    the sequential order that process_options() claims to strictly follow
>>    (the PID file is only created after all arguments are processed, not
>>    at the time the --pidfile argument appears), so it makes sense to
>>    include it in the same category as --daemonize.
>>
>> - Invalid arguments should always be reported as soon as possible.  (The
>>    same caveat with --help applies: That means that "--blockdev inv-drv
>>    --inv-arg" will now complain about --inv-arg, not inv-drv.)
>>
>> Note that we could decide to check only for --daemonize in the first
>> pass, and defer --help, --version, and checking for invalid arguments to
>> the second one, thus largely keeping our current behavior. However,
>> this would break "--help --daemonize": The child would print the help
>> text to stdout, which is redirected to /dev/null, and so the text would
>> disappear.  We would need to have the text be printed to stderr instead,
>> and this would then make the parent process exit with EXIT_FAILURE,
>> which is probably not what we want for --help.
>>
>> This patch does make some references to --daemonize without having
>> implemented it yet, but that will happen in the next patch.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> ---
>>   storage-daemon/qemu-storage-daemon.c | 37 ++++++++++++++++++++++++++--
>>   1 file changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/storage-daemon/qemu-storage-daemon.c 
>> b/storage-daemon/qemu-storage-daemon.c
>> index 52cf17e8ac..42a52d3b1c 100644
>> --- a/storage-daemon/qemu-storage-daemon.c
>> +++ b/storage-daemon/qemu-storage-daemon.c
>> @@ -164,7 +164,23 @@ static int getopt_set_loc(int argc, char **argv, 
>> const char *optstring,
>>       return c;
>>   }
>>   -static void process_options(int argc, char *argv[])
>> +/**
>> + * Process QSD command-line arguments.
>> + *
>> + * This is done in two passes:
>> + *
>> + * First (@pre_init_pass is true), we do a pass where all global
>> + * arguments pertaining to the QSD process (like --help or --daemonize)
>> + * are processed.  This pass is done before most of the QEMU-specific
>> + * initialization steps (e.g. initializing the block layer or QMP), and
>> + * so must only process arguments that are not really QEMU-specific.
>> + *
>> + * Second (@pre_init_pass is false), we (sequentially) process all
>> + * QEMU/QSD-specific arguments.  Many of these arguments are 
>> effectively
>> + * translated to QMP commands (like --blockdev for blockdev-add, or
>> + * --export for block-export-add).
>> + */
>> +static void process_options(int argc, char *argv[], bool pre_init_pass)
>>   {
>>       int c;
>>   @@ -187,7 +203,22 @@ static void process_options(int argc, char 
>> *argv[])
>>        * they are given on the command lines. This means that things 
>> must be
>
> So, --pidfile already breaks a bit this comment. Still would be good 
> to adjust it now..
>
> may be, s/options/QEMU-specific options/ or something like this.

Right, makes sense to make it match the comment above the function.

>>        * defined first before they can be referenced in another option.
>>        */
>> +    optind = 1;
>>       while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) 
>> != -1) {
>> +        bool handle_option_pre_init;
>> +
>> +        /* Should this argument be processed in the pre-init pass? */
>> +        handle_option_pre_init =
>> +            c == '?' ||
>> +            c == 'h' ||
>> +            c == 'V' ||
>> +            c == OPTION_PIDFILE;
>> +
>> +        /* Process every option only in its respective pass */
>> +        if (pre_init_pass != handle_option_pre_init) {
>> +            continue;
>> +        }
>> +
>>           switch (c) {
>>           case '?':
>>               exit(EXIT_FAILURE);
>> @@ -321,6 +352,8 @@ int main(int argc, char *argv[])
>>       qemu_init_exec_dir(argv[0]);
>>       os_setup_signal_handling();
>>   +    process_options(argc, argv, true);
>> +
>>       module_call_init(MODULE_INIT_QOM);
>>       module_call_init(MODULE_INIT_TRACE);
>>       qemu_add_opts(&qemu_trace_opts);
>> @@ -335,7 +368,7 @@ int main(int argc, char *argv[])
>>       qemu_set_log(LOG_TRACE);
>>         qemu_init_main_loop(&error_fatal);
>> -    process_options(argc, argv);
>> +    process_options(argc, argv, false);
>>         /*
>>        * Write the pid file after creating chardevs, exports, and NBD 
>> servers but
>>
>
>

Re: [PATCH 2/3] qsd: Add --daemonize

[Hanna Reitz]

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

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Markus Armbruster]

Hanna Reitz <hreitz@redhat.com> writes:

> We want to add a --daemonize argument to QSD's command line.

Why?

>                                                               This will
> require forking the process before we do any complex initialization
> steps, like setting up the block layer or QMP.  Therefore, we must scan
> the command line for it long before our current process_options() call.

Can you explain in a bit more detail why early forking is required?

I have a strong dislike for parsing more than once...

> Instead of adding custom new code to do so, just reuse process_options()
> and give it a @pre_init_pass argument to distinguish the two passes.  I
> believe there are some other switches but --daemonize that deserve
> parsing in the first pass:
>
> - --help and --version are supposed to only print some text and then
>   immediately exit (so any initialization we do would be for naught).
>   This changes behavior, because now "--blockdev inv-drv --help" will
>   print a help text instead of complaining about the --blockdev
>   argument.
>   Note that this is similar in behavior to other tools, though: "--help"
>   is generally immediately acted upon when finding it in the argument
>   list, potentially before other arguments (even ones before it) are
>   acted on.  For example, "ls /does-not-exist --help" prints a help text
>   and does not complain about ENOENT.
>
> - --pidfile does not need initialization, and is already exempted from
>   the sequential order that process_options() claims to strictly follow
>   (the PID file is only created after all arguments are processed, not
>   at the time the --pidfile argument appears), so it makes sense to
>   include it in the same category as --daemonize.
>
> - Invalid arguments should always be reported as soon as possible.  (The
>   same caveat with --help applies: That means that "--blockdev inv-drv
>   --inv-arg" will now complain about --inv-arg, not inv-drv.)
>
> Note that we could decide to check only for --daemonize in the first
> pass, and defer --help, --version, and checking for invalid arguments to
> the second one, thus largely keeping our current behavior.  However,
> this would break "--help --daemonize": The child would print the help
> text to stdout, which is redirected to /dev/null, and so the text would
> disappear.  We would need to have the text be printed to stderr instead,
> and this would then make the parent process exit with EXIT_FAILURE,
> which is probably not what we want for --help.
>
> This patch does make some references to --daemonize without having
> implemented it yet, but that will happen in the next patch.
>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

On 19.01.22 13:58, Markus Armbruster wrote:
> Hanna Reitz <hreitz@redhat.com> writes:
>
>> We want to add a --daemonize argument to QSD's command line.
> Why?

OK, s/we/I/.  I find it useful, because without such an option, I need 
to have whoever invokes QSD loop until the PID file exists, before I can 
be sure that all exports are set up.  I make use of it in the test cases 
added in patch 3.

I suppose this could be worked around with a special character device, 
like so:

```
ncat --listen -U /tmp/qsd-done.sock </dev/null &
ncat_pid=$!

qemu-storage-daemon \
     ... \
     --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
     --monitor signal_done \
     --pidfile /tmp/qsd.pid &

wait $ncat_pid
```

But having to use an extra tool for this is unergonomic.  I mean, if 
there’s no other way...

>>                                                                This will
>> require forking the process before we do any complex initialization
>> steps, like setting up the block layer or QMP.  Therefore, we must scan
>> the command line for it long before our current process_options() call.
> Can you explain in a bit more detail why early forking is required?
>
> I have a strong dislike for parsing more than once...

Because I don’t want to set up QMP and block devices, and then fork the 
process into two.  That sounds like there’d be a lot of stuff to think 
about, which just isn’t necessary, because we don’t need to set up any 
of this in the parent.

For example, if I set up a monitor on a Unix socket (server=true), 
processing is delayed until the client connects.  Say I put --daemonize 
afterwards.  I connect to the waiting server socket, the child is forked 
off, and then... I’m not sure what happens, actually.  Do I have a 
connection with both the parent and the child listening?  I know that in 
practice, what happens is that once the parent exits, the connection is 
closed, and I get a “qemu: qemu_thread_join: Invalid argument” 
warning/error on the QSD side.

There’s a lot of stuff to think about if you allow forking after other 
options, so it should be done first.  We could just require the user to 
put --daemonize before all other options, and so have a single pass; but 
still, before options are even parsed, we have already for example 
called bdrv_init(), init_qmp_commands(), qemu_init_main_loop().  These 
are all things that the parent of a daemonizing process doesn’t need to 
do, and where I’d simply rather not think about what impact it has if we 
fork afterwards.

Hanna

>> Instead of adding custom new code to do so, just reuse process_options()
>> and give it a @pre_init_pass argument to distinguish the two passes.  I
>> believe there are some other switches but --daemonize that deserve
>> parsing in the first pass:
>>
>> - --help and --version are supposed to only print some text and then
>>    immediately exit (so any initialization we do would be for naught).
>>    This changes behavior, because now "--blockdev inv-drv --help" will
>>    print a help text instead of complaining about the --blockdev
>>    argument.
>>    Note that this is similar in behavior to other tools, though: "--help"
>>    is generally immediately acted upon when finding it in the argument
>>    list, potentially before other arguments (even ones before it) are
>>    acted on.  For example, "ls /does-not-exist --help" prints a help text
>>    and does not complain about ENOENT.
>>
>> - --pidfile does not need initialization, and is already exempted from
>>    the sequential order that process_options() claims to strictly follow
>>    (the PID file is only created after all arguments are processed, not
>>    at the time the --pidfile argument appears), so it makes sense to
>>    include it in the same category as --daemonize.
>>
>> - Invalid arguments should always be reported as soon as possible.  (The
>>    same caveat with --help applies: That means that "--blockdev inv-drv
>>    --inv-arg" will now complain about --inv-arg, not inv-drv.)
>>
>> Note that we could decide to check only for --daemonize in the first
>> pass, and defer --help, --version, and checking for invalid arguments to
>> the second one, thus largely keeping our current behavior.  However,
>> this would break "--help --daemonize": The child would print the help
>> text to stdout, which is redirected to /dev/null, and so the text would
>> disappear.  We would need to have the text be printed to stderr instead,
>> and this would then make the parent process exit with EXIT_FAILURE,
>> which is probably not what we want for --help.
>>
>> This patch does make some references to --daemonize without having
>> implemented it yet, but that will happen in the next patch.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Kevin Wolf]

Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
> On 19.01.22 13:58, Markus Armbruster wrote:
> > Hanna Reitz <hreitz@redhat.com> writes:
> > 
> > > We want to add a --daemonize argument to QSD's command line.
> > Why?
> 
> OK, s/we/I/.  I find it useful, because without such an option, I need to
> have whoever invokes QSD loop until the PID file exists, before I can be
> sure that all exports are set up.  I make use of it in the test cases added
> in patch 3.
> 
> I suppose this could be worked around with a special character device, like
> so:
> 
> ```
> ncat --listen -U /tmp/qsd-done.sock </dev/null &
> ncat_pid=$!
> 
> qemu-storage-daemon \
>     ... \
>     --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>     --monitor signal_done \
>     --pidfile /tmp/qsd.pid &
> 
> wait $ncat_pid
> ```
> 
> But having to use an extra tool for this is unergonomic.  I mean, if there’s
> no other way...

The other point is that the system emulator has it, qemu-nbd has it,
so certainly qsd should have it as well. Not the least because it should
be able to replace qemu-nbd (at least for the purpose of exporting NBD.
not necessarily for attaching it to the host).

> > >                                                                This will
> > > require forking the process before we do any complex initialization
> > > steps, like setting up the block layer or QMP.  Therefore, we must scan
> > > the command line for it long before our current process_options() call.
> > Can you explain in a bit more detail why early forking is required?
> > 
> > I have a strong dislike for parsing more than once...
> 
> Because I don’t want to set up QMP and block devices, and then fork the
> process into two.  That sounds like there’d be a lot of stuff to think
> about, which just isn’t necessary, because we don’t need to set up any
> of this in the parent.

Here we can compare again: Both the system emulator and qemu-nbd behave
the same, they fork before they do anything interesting.

The difference is that they still parse the command line only once
because they don't immediately create things, but just store the options
and later process them in their own magic order. I'd much rather parse
the command line twice than copy that behaviour.

Kevin

> For example, if I set up a monitor on a Unix socket (server=true),
> processing is delayed until the client connects.  Say I put --daemonize
> afterwards.  I connect to the waiting server socket, the child is forked
> off, and then... I’m not sure what happens, actually.  Do I have a
> connection with both the parent and the child listening?  I know that in
> practice, what happens is that once the parent exits, the connection is
> closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error
> on the QSD side.
> 
> There’s a lot of stuff to think about if you allow forking after other
> options, so it should be done first.  We could just require the user to put
> --daemonize before all other options, and so have a single pass; but still,
> before options are even parsed, we have already for example called
> bdrv_init(), init_qmp_commands(), qemu_init_main_loop().  These are all
> things that the parent of a daemonizing process doesn’t need to do, and
> where I’d simply rather not think about what impact it has if we fork
> afterwards.
> 
> Hanna
> 
> > > Instead of adding custom new code to do so, just reuse process_options()
> > > and give it a @pre_init_pass argument to distinguish the two passes.  I
> > > believe there are some other switches but --daemonize that deserve
> > > parsing in the first pass:
> > > 
> > > - --help and --version are supposed to only print some text and then
> > >    immediately exit (so any initialization we do would be for naught).
> > >    This changes behavior, because now "--blockdev inv-drv --help" will
> > >    print a help text instead of complaining about the --blockdev
> > >    argument.
> > >    Note that this is similar in behavior to other tools, though: "--help"
> > >    is generally immediately acted upon when finding it in the argument
> > >    list, potentially before other arguments (even ones before it) are
> > >    acted on.  For example, "ls /does-not-exist --help" prints a help text
> > >    and does not complain about ENOENT.
> > > 
> > > - --pidfile does not need initialization, and is already exempted from
> > >    the sequential order that process_options() claims to strictly follow
> > >    (the PID file is only created after all arguments are processed, not
> > >    at the time the --pidfile argument appears), so it makes sense to
> > >    include it in the same category as --daemonize.
> > > 
> > > - Invalid arguments should always be reported as soon as possible.  (The
> > >    same caveat with --help applies: That means that "--blockdev inv-drv
> > >    --inv-arg" will now complain about --inv-arg, not inv-drv.)
> > > 
> > > Note that we could decide to check only for --daemonize in the first
> > > pass, and defer --help, --version, and checking for invalid arguments to
> > > the second one, thus largely keeping our current behavior.  However,
> > > this would break "--help --daemonize": The child would print the help
> > > text to stdout, which is redirected to /dev/null, and so the text would
> > > disappear.  We would need to have the text be printed to stderr instead,
> > > and this would then make the parent process exit with EXIT_FAILURE,
> > > which is probably not what we want for --help.
> > > 
> > > This patch does make some references to --daemonize without having
> > > implemented it yet, but that will happen in the next patch.
> > > 
> > > Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> 

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
>> On 19.01.22 13:58, Markus Armbruster wrote:
>> > Hanna Reitz <hreitz@redhat.com> writes:
>> > 
>> > > We want to add a --daemonize argument to QSD's command line.
>> > 
>> > Why?
>> 
>> OK, s/we/I/.  I find it useful, because without such an option, I need to
>> have whoever invokes QSD loop until the PID file exists, before I can be
>> sure that all exports are set up.  I make use of it in the test cases added
>> in patch 3.
>> 
>> I suppose this could be worked around with a special character device, like
>> so:
>> 
>> ```
>> ncat --listen -U /tmp/qsd-done.sock </dev/null &
>> ncat_pid=$!
>> 
>> qemu-storage-daemon \
>>     ... \
>>     --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>>     --monitor signal_done \
>>     --pidfile /tmp/qsd.pid &
>> 
>> wait $ncat_pid
>> ```
>> 
>> But having to use an extra tool for this is unergonomic.  I mean, if there’s
>> no other way...

I know duplicating this into every program that could server as a daemon
is the Unix tradition.  Doesn't make it good.  Systemd[*] has tried to
make it superfluous.

> The other point is that the system emulator has it, qemu-nbd has it,
> so certainly qsd should have it as well. Not the least because it should
> be able to replace qemu-nbd (at least for the purpose of exporting NBD.
> not necessarily for attaching it to the host).

Point taken, but I think it's a somewhat weak one.  qsd could certainly
replace qemu-nbd even without --daemonize; we could use other means to
run it in the background.

>> > >                                                                This will
>> > > require forking the process before we do any complex initialization
>> > > steps, like setting up the block layer or QMP.  Therefore, we must scan
>> > > the command line for it long before our current process_options() call.
>> > 
>> > Can you explain in a bit more detail why early forking is required?
>> > 
>> > I have a strong dislike for parsing more than once...
>> 
>> Because I don’t want to set up QMP and block devices, and then fork the
>> process into two.  That sounds like there’d be a lot of stuff to think
>> about, which just isn’t necessary, because we don’t need to set up any
>> of this in the parent.

We must fork() before we create threads.  Other resources are easy
enough to hand over to the child.  Still, having to think about less is
good, I readily grant you that.

The trouble is that forking early creates a new problem: any
configuration errors detected in the child must be propagated to the
parent somehow (output and exit status).  I peeked at your PATCH 2, and
I'm not convinced, but that's detail here.

> Here we can compare again: Both the system emulator and qemu-nbd behave
> the same, they fork before they do anything interesting.
>
> The difference is that they still parse the command line only once
> because they don't immediately create things, but just store the options
> and later process them in their own magic order. I'd much rather parse
> the command line twice than copy that behaviour.

The part I hate is "own magic order".  Without that, multiple passes are
just fine with me.

Parsing twice is a bit like having a two pass compiler run the first
pass left to right, and then both passes intertwined left to right.  The
pedestrian way to do it is running the first pass left to right, then
the second pass left to right.

We're clearly talking taste here.

>
> Kevin
>
>> For example, if I set up a monitor on a Unix socket (server=true),
>> processing is delayed until the client connects.  Say I put --daemonize
>> afterwards.  I connect to the waiting server socket, the child is forked
>> off, and then... I’m not sure what happens, actually.  Do I have a
>> connection with both the parent and the child listening?  I know that in
>> practice, what happens is that once the parent exits, the connection is
>> closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error
>> on the QSD side.
>> 
>> There’s a lot of stuff to think about if you allow forking after other
>> options, so it should be done first.  We could just require the user to put
>> --daemonize before all other options, and so have a single pass; but still,
>> before options are even parsed, we have already for example called
>> bdrv_init(), init_qmp_commands(), qemu_init_main_loop().  These are all
>> things that the parent of a daemonizing process doesn’t need to do, and
>> where I’d simply rather not think about what impact it has if we fork
>> afterwards.
>> 
>> Hanna

Care to put a brief version of the rationale for --daemonize and for
forking early in the commit message?

[...]


[*] Not everything systemd does is bad.  It's a big, mixed bag of ideas.

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

On 20.01.22 17:00, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
>>> On 19.01.22 13:58, Markus Armbruster wrote:
>>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>>
>>>>> We want to add a --daemonize argument to QSD's command line.
>>>> Why?
>>> OK, s/we/I/.  I find it useful, because without such an option, I need to
>>> have whoever invokes QSD loop until the PID file exists, before I can be
>>> sure that all exports are set up.  I make use of it in the test cases added
>>> in patch 3.
>>>
>>> I suppose this could be worked around with a special character device, like
>>> so:
>>>
>>> ```
>>> ncat --listen -U /tmp/qsd-done.sock </dev/null &
>>> ncat_pid=$!
>>>
>>> qemu-storage-daemon \
>>>      ... \
>>>      --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>>>      --monitor signal_done \
>>>      --pidfile /tmp/qsd.pid &
>>>
>>> wait $ncat_pid
>>> ```
>>>
>>> But having to use an extra tool for this is unergonomic.  I mean, if there’s
>>> no other way...
> I know duplicating this into every program that could server as a daemon
> is the Unix tradition.  Doesn't make it good.  Systemd[*] has tried to
> make it superfluous.

Well.  I have absolutely nothing against systemd.  Still, I will not use 
it in an iotest, that’s for sure.

>> The other point is that the system emulator has it, qemu-nbd has it,
>> so certainly qsd should have it as well. Not the least because it should
>> be able to replace qemu-nbd (at least for the purpose of exporting NBD.
>> not necessarily for attaching it to the host).
> Point taken, but I think it's a somewhat weak one.  qsd could certainly
> replace qemu-nbd even without --daemonize; we could use other means to
> run it in the background.
>
>>>>>                                                                 This will
>>>>> require forking the process before we do any complex initialization
>>>>> steps, like setting up the block layer or QMP.  Therefore, we must scan
>>>>> the command line for it long before our current process_options() call.
>>>> Can you explain in a bit more detail why early forking is required?
>>>>
>>>> I have a strong dislike for parsing more than once...
>>> Because I don’t want to set up QMP and block devices, and then fork the
>>> process into two.  That sounds like there’d be a lot of stuff to think
>>> about, which just isn’t necessary, because we don’t need to set up any
>>> of this in the parent.
> We must fork() before we create threads.  Other resources are easy
> enough to hand over to the child.  Still, having to think about less is
> good, I readily grant you that.
>
> The trouble is that forking early creates a new problem: any
> configuration errors detected in the child must be propagated to the
> parent somehow (output and exit status).  I peeked at your PATCH 2, and
> I'm not convinced, but that's detail here.
>
>> Here we can compare again: Both the system emulator and qemu-nbd behave
>> the same, they fork before they do anything interesting.
>>
>> The difference is that they still parse the command line only once
>> because they don't immediately create things, but just store the options
>> and later process them in their own magic order. I'd much rather parse
>> the command line twice than copy that behaviour.
> The part I hate is "own magic order".  Without that, multiple passes are
> just fine with me.
>
> Parsing twice is a bit like having a two pass compiler run the first
> pass left to right, and then both passes intertwined left to right.  The
> pedestrian way to do it is running the first pass left to right, then
> the second pass left to right.
>
> We're clearly talking taste here.
>
>> Kevin
>>
>>> For example, if I set up a monitor on a Unix socket (server=true),
>>> processing is delayed until the client connects.  Say I put --daemonize
>>> afterwards.  I connect to the waiting server socket, the child is forked
>>> off, and then... I’m not sure what happens, actually.  Do I have a
>>> connection with both the parent and the child listening?  I know that in
>>> practice, what happens is that once the parent exits, the connection is
>>> closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error
>>> on the QSD side.
>>>
>>> There’s a lot of stuff to think about if you allow forking after other
>>> options, so it should be done first.  We could just require the user to put
>>> --daemonize before all other options, and so have a single pass; but still,
>>> before options are even parsed, we have already for example called
>>> bdrv_init(), init_qmp_commands(), qemu_init_main_loop().  These are all
>>> things that the parent of a daemonizing process doesn’t need to do, and
>>> where I’d simply rather not think about what impact it has if we fork
>>> afterwards.
>>>
>>> Hanna
> Care to put a brief version of the rationale for --daemonize and for
> forking early in the commit message?

Well, my rationale for adding the feature doesn’t really extend beyond 
“I want it, I find it useful, and so I assume others will, too”.

I don’t really like putting “qemu-nbd has it” there, because... it was 
again me who implemented it for qemu-nbd.  Because I found it useful.  
But I can of course do that, if it counts as a reason.

I can certainly (and understand the need to, and will) elaborate on the 
“This will require forking the process before we do any complex 
initialization steps” part.

Hanna

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Markus Armbruster]

Hanna Reitz <hreitz@redhat.com> writes:

> On 20.01.22 17:00, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
>>>> On 19.01.22 13:58, Markus Armbruster wrote:
>>>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>>>
>>>>>> We want to add a --daemonize argument to QSD's command line.
>>>>> Why?
>>>> OK, s/we/I/.  I find it useful, because without such an option, I need to
>>>> have whoever invokes QSD loop until the PID file exists, before I can be
>>>> sure that all exports are set up.  I make use of it in the test cases added
>>>> in patch 3.
>>>>
>>>> I suppose this could be worked around with a special character device, like
>>>> so:
>>>>
>>>> ```
>>>> ncat --listen -U /tmp/qsd-done.sock </dev/null &
>>>> ncat_pid=$!
>>>>
>>>> qemu-storage-daemon \
>>>>      ... \
>>>>      --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>>>>      --monitor signal_done \
>>>>      --pidfile /tmp/qsd.pid &
>>>>
>>>> wait $ncat_pid
>>>> ```
>>>>
>>>> But having to use an extra tool for this is unergonomic.  I mean, if there’s
>>>> no other way...
>>
>> I know duplicating this into every program that could server as a daemon
>> is the Unix tradition.  Doesn't make it good.  Systemd[*] has tried to
>> make it superfluous.
>
> Well.  I have absolutely nothing against systemd.  Still, I will not
> use it in an iotest, that’s for sure.

My point isn't "use systemd in iotests".  It's "consider doing it like
systemd", i.e. do the daemonization work in a utility program.  For what
it's worth, Linux has daemonize(1).

[...]

>> Care to put a brief version of the rationale for --daemonize and for
>> forking early in the commit message?
>
> Well, my rationale for adding the feature doesn’t really extend beyond
> “I want it, I find it useful, and so I assume others will, too”.

Don't pretend to be obtuse, it's not credible :)  You mentioned iotests,
which makes me guess your rationale is "I want this for iotests, and
there may well be other uses."

> I don’t really like putting “qemu-nbd has it” there, because... it was
> again me who implemented it for qemu-nbd.  Because I found it useful.  
> But I can of course do that, if it counts as a reason.

Useful *what for*, and we have rationale.

> I can certainly (and understand the need to, and will) elaborate on
> the “This will require forking the process before we do any complex 
> initialization steps” part.

Thanks!

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

On 21.01.22 07:10, Markus Armbruster wrote:
> Hanna Reitz <hreitz@redhat.com> writes:
>
>> On 20.01.22 17:00, Markus Armbruster wrote:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
>>>>> On 19.01.22 13:58, Markus Armbruster wrote:
>>>>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>>>>
>>>>>>> We want to add a --daemonize argument to QSD's command line.
>>>>>> Why?
>>>>> OK, s/we/I/.  I find it useful, because without such an option, I need to
>>>>> have whoever invokes QSD loop until the PID file exists, before I can be
>>>>> sure that all exports are set up.  I make use of it in the test cases added
>>>>> in patch 3.
>>>>>
>>>>> I suppose this could be worked around with a special character device, like
>>>>> so:
>>>>>
>>>>> ```
>>>>> ncat --listen -U /tmp/qsd-done.sock </dev/null &
>>>>> ncat_pid=$!
>>>>>
>>>>> qemu-storage-daemon \
>>>>>       ... \
>>>>>       --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>>>>>       --monitor signal_done \
>>>>>       --pidfile /tmp/qsd.pid &
>>>>>
>>>>> wait $ncat_pid
>>>>> ```
>>>>>
>>>>> But having to use an extra tool for this is unergonomic.  I mean, if there’s
>>>>> no other way...
>>> I know duplicating this into every program that could server as a daemon
>>> is the Unix tradition.  Doesn't make it good.  Systemd[*] has tried to
>>> make it superfluous.
>> Well.  I have absolutely nothing against systemd.  Still, I will not
>> use it in an iotest, that’s for sure.
> My point isn't "use systemd in iotests".  It's "consider doing it like
> systemd", i.e. do the daemonization work in a utility program.  For what
> it's worth, Linux has daemonize(1).

The problem I face is that currently there is no ergonomic way to wait 
until the QSD is up and running (besides looping until the PID file 
exists), and I don’t think a utility program that doesn’t know the QSD 
could provide this.  (For example, it looks like daemonize(1) will have 
the parent exit immediately, regardless of whether the child is set up 
or not.)

> [...]
>
>>> Care to put a brief version of the rationale for --daemonize and for
>>> forking early in the commit message?
>> Well, my rationale for adding the feature doesn’t really extend beyond
>> “I want it, I find it useful, and so I assume others will, too”.
> Don't pretend to be obtuse, it's not credible :)  You mentioned iotests,
> which makes me guess your rationale is "I want this for iotests, and
> there may well be other uses."

Oh, I also want it for other things, like the script I have to use the 
QSD to make disk images accessible as raw files.  Thing is, the stress 
is on “want” in contrast to “need”.  I can do without --daemonize, I 
have already done so, even before there was --pidfile (I just queried 
the block exports through QMP until they were all there).  It’s just 
that it’s kind of a pain.

Same with the iotests, it’s absolutely possible to get away without 
--daemonize.  It’s just that I wrote the test, wanted to use some form 
of --daemonize option, noticed there wasn’t any yet, and thought “Oh, 
that’d be nice to have”.

I would love a --daemonize option, but I can’t say it’s necessary. If 
the way it’d need to be implemented isn’t acceptable, then I won’t force 
it into the code.

>> I don’t really like putting “qemu-nbd has it” there, because... it was
>> again me who implemented it for qemu-nbd.  Because I found it useful.
>> But I can of course do that, if it counts as a reason.
> Useful *what for*, and we have rationale.
>
>> I can certainly (and understand the need to, and will) elaborate on
>> the “This will require forking the process before we do any complex
>> initialization steps” part.
> Thanks!
>

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Markus Armbruster]

Hanna Reitz <hreitz@redhat.com> writes:

> On 21.01.22 07:10, Markus Armbruster wrote:
>> Hanna Reitz <hreitz@redhat.com> writes:
>>
>>> On 20.01.22 17:00, Markus Armbruster wrote:
>>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>>
>>>>> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
>>>>>> On 19.01.22 13:58, Markus Armbruster wrote:
>>>>>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>>>>>
>>>>>>>> We want to add a --daemonize argument to QSD's command line.
>>>>>>> Why?
>>>>>> OK, s/we/I/.  I find it useful, because without such an option, I need to
>>>>>> have whoever invokes QSD loop until the PID file exists, before I can be
>>>>>> sure that all exports are set up.  I make use of it in the test cases added
>>>>>> in patch 3.
>>>>>>
>>>>>> I suppose this could be worked around with a special character device, like
>>>>>> so:
>>>>>>
>>>>>> ```
>>>>>> ncat --listen -U /tmp/qsd-done.sock </dev/null &
>>>>>> ncat_pid=$!
>>>>>>
>>>>>> qemu-storage-daemon \
>>>>>>       ... \
>>>>>>       --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>>>>>>       --monitor signal_done \
>>>>>>       --pidfile /tmp/qsd.pid &
>>>>>>
>>>>>> wait $ncat_pid
>>>>>> ```
>>>>>>
>>>>>> But having to use an extra tool for this is unergonomic.  I mean, if there’s
>>>>>> no other way...
>>>> I know duplicating this into every program that could server as a daemon
>>>> is the Unix tradition.  Doesn't make it good.  Systemd[*] has tried to
>>>> make it superfluous.
>>>
>>> Well.  I have absolutely nothing against systemd.  Still, I will not
>>> use it in an iotest, that’s for sure.
>
>> My point isn't "use systemd in iotests".  It's "consider doing it like
>> systemd", i.e. do the daemonization work in a utility program.  For what
>> it's worth, Linux has daemonize(1).
>
> The problem I face is that currently there is no ergonomic way to wait
> until the QSD is up and running (besides looping until the PID file 
> exists), and I don’t think a utility program that doesn’t know the QSD
> could provide this.  (For example, it looks like daemonize(1) will
> have the parent exit immediately, regardless of whether the child is
> set up or not.)

Why do you need to wait for QSD to be ready?

I'm asking because with common daemons, I don't wait, I just connect to
their socket and start talking.  They'll reply only when ready.

>> [...]
>>
>>>> Care to put a brief version of the rationale for --daemonize and for
>>>> forking early in the commit message?
>>>
>>> Well, my rationale for adding the feature doesn’t really extend beyond
>>> “I want it, I find it useful, and so I assume others will, too”.
>>>
>> Don't pretend to be obtuse, it's not credible :)  You mentioned iotests,
>> which makes me guess your rationale is "I want this for iotests, and
>> there may well be other uses."
>
> Oh, I also want it for other things, like the script I have to use the
> QSD to make disk images accessible as raw files.  Thing is, the stress 
> is on “want” in contrast to “need”.  I can do without --daemonize, I
> have already done so, even before there was --pidfile (I just queried 
> the block exports through QMP until they were all there).  It’s just
> that it’s kind of a pain.
>
> Same with the iotests, it’s absolutely possible to get away without
> --daemonize.  It’s just that I wrote the test, wanted to use some form 
> of --daemonize option, noticed there wasn’t any yet, and thought “Oh,
> that’d be nice to have”.
>
> I would love a --daemonize option, but I can’t say it’s necessary. If
> the way it’d need to be implemented isn’t acceptable, then I won’t
> force it into the code.

Rationale doesn't have to be "we must have this because".  It can also
be "I want this because".  What it can't be is "I want this".

[...]

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

On 21.01.22 11:27, Markus Armbruster wrote:
> Hanna Reitz <hreitz@redhat.com> writes:
>
>> On 21.01.22 07:10, Markus Armbruster wrote:
>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>
>>>> On 20.01.22 17:00, Markus Armbruster wrote:
>>>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>>>
>>>>>> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
>>>>>>> On 19.01.22 13:58, Markus Armbruster wrote:
>>>>>>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>>>>>>
>>>>>>>>> We want to add a --daemonize argument to QSD's command line.
>>>>>>>> Why?
>>>>>>> OK, s/we/I/.  I find it useful, because without such an option, I need to
>>>>>>> have whoever invokes QSD loop until the PID file exists, before I can be
>>>>>>> sure that all exports are set up.  I make use of it in the test cases added
>>>>>>> in patch 3.
>>>>>>>
>>>>>>> I suppose this could be worked around with a special character device, like
>>>>>>> so:
>>>>>>>
>>>>>>> ```
>>>>>>> ncat --listen -U /tmp/qsd-done.sock </dev/null &
>>>>>>> ncat_pid=$!
>>>>>>>
>>>>>>> qemu-storage-daemon \
>>>>>>>        ... \
>>>>>>>        --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>>>>>>>        --monitor signal_done \
>>>>>>>        --pidfile /tmp/qsd.pid &
>>>>>>>
>>>>>>> wait $ncat_pid
>>>>>>> ```
>>>>>>>
>>>>>>> But having to use an extra tool for this is unergonomic.  I mean, if there’s
>>>>>>> no other way...
>>>>> I know duplicating this into every program that could server as a daemon
>>>>> is the Unix tradition.  Doesn't make it good.  Systemd[*] has tried to
>>>>> make it superfluous.
>>>> Well.  I have absolutely nothing against systemd.  Still, I will not
>>>> use it in an iotest, that’s for sure.
>>> My point isn't "use systemd in iotests".  It's "consider doing it like
>>> systemd", i.e. do the daemonization work in a utility program.  For what
>>> it's worth, Linux has daemonize(1).
>> The problem I face is that currently there is no ergonomic way to wait
>> until the QSD is up and running (besides looping until the PID file
>> exists), and I don’t think a utility program that doesn’t know the QSD
>> could provide this.  (For example, it looks like daemonize(1) will
>> have the parent exit immediately, regardless of whether the child is
>> set up or not.)
> Why do you need to wait for QSD to be ready?
>
> I'm asking because with common daemons, I don't wait, I just connect to
> their socket and start talking.  They'll reply only when ready.

That only applies when you want to talk to a socket, which I often don’t 
do.  Most of the time I use the storage daemon, I pass all --blockdev 
and --export options through the command line and don’t create any 
socket at all.  When I use the QSD just to export some block device, I 
generally don’t need QMP.

Of course, I could just not do that, and instead only set up QMP and 
then do all the configuration through that (where, as you say, QSD will 
only reply once it can); but that’s much more complicated than running a 
single command.

(Or I could do a mix of both, which I described above, where I’d create 
and have the QSD connect to a Unix socket just to see that configuration 
is done and all exports are up.  I’d prefer not to, because it still 
means using an extra tool (ncat) to create the socket.)

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Markus Armbruster]

Hanna Reitz <hreitz@redhat.com> writes:

> On 21.01.22 11:27, Markus Armbruster wrote:
>> Hanna Reitz <hreitz@redhat.com> writes:
>>> The problem I face is that currently there is no ergonomic way to wait
>>> until the QSD is up and running (besides looping until the PID file
>>> exists), and I don’t think a utility program that doesn’t know the QSD
>>> could provide this.  (For example, it looks like daemonize(1) will
>>> have the parent exit immediately, regardless of whether the child is
>>> set up or not.)
>>
>> Why do you need to wait for QSD to be ready?
>>
>> I'm asking because with common daemons, I don't wait, I just connect to
>> their socket and start talking.  They'll reply only when ready.
>
> That only applies when you want to talk to a socket, which I often
> don’t do.  Most of the time I use the storage daemon, I pass all
> --blockdev and --export options through the command line and don’t
>  create any socket at all.  When I use the QSD just to export some
> block device, I generally don’t need QMP.

If you export via NBD, why can't you just connect to NBD socket?

> Of course, I could just not do that, and instead only set up QMP and
> then do all the configuration through that (where, as you say, QSD
> will only reply once it can); but that’s much more complicated than
> running a single command.
>
> (Or I could do a mix of both, which I described above, where I’d
> create and have the QSD connect to a Unix socket just to see that
> configuration is done and all exports are up.  I’d prefer not to,
> because it still means using an extra tool (ncat) to create the
> socket.)

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

On 21.01.22 15:26, Markus Armbruster wrote:
> Hanna Reitz <hreitz@redhat.com> writes:
>
>> On 21.01.22 11:27, Markus Armbruster wrote:
>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>> The problem I face is that currently there is no ergonomic way to wait
>>>> until the QSD is up and running (besides looping until the PID file
>>>> exists), and I don’t think a utility program that doesn’t know the QSD
>>>> could provide this.  (For example, it looks like daemonize(1) will
>>>> have the parent exit immediately, regardless of whether the child is
>>>> set up or not.)
>>> Why do you need to wait for QSD to be ready?
>>>
>>> I'm asking because with common daemons, I don't wait, I just connect to
>>> their socket and start talking.  They'll reply only when ready.
>> That only applies when you want to talk to a socket, which I often
>> don’t do.  Most of the time I use the storage daemon, I pass all
>> --blockdev and --export options through the command line and don’t
>>   create any socket at all.  When I use the QSD just to export some
>> block device, I generally don’t need QMP.
> If you export via NBD, why can't you just connect to NBD socket?

I’m not sure what exactly you mean by this, because the socket doesn’t 
exist before the QSD is launched.  If I launch the QSD in the background 
and have it create an NBD server on a Unix socket, then this socket will 
not exist until the respective --nbd-server option is parsed.  Trying to 
connect to it immediately after the QSD has been launched may work (if 
the QSD was quicker to parse the option and create the server than me 
trying to connect) or may yield ECONNREFUSED or ENOENT, depending on 
whether the socket file existed before or not.

Also, outside of the iotests, I personally generally usually use FUSE 
exports instead of NBD exports.

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Markus Armbruster]

Hanna Reitz <hreitz@redhat.com> writes:

> On 21.01.22 15:26, Markus Armbruster wrote:
>> Hanna Reitz <hreitz@redhat.com> writes:
>>
>>> On 21.01.22 11:27, Markus Armbruster wrote:
>>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>>> The problem I face is that currently there is no ergonomic way to wait
>>>>> until the QSD is up and running (besides looping until the PID file
>>>>> exists), and I don’t think a utility program that doesn’t know the QSD
>>>>> could provide this.  (For example, it looks like daemonize(1) will
>>>>> have the parent exit immediately, regardless of whether the child is
>>>>> set up or not.)
>>>>
>>>> Why do you need to wait for QSD to be ready?
>>>>
>>>> I'm asking because with common daemons, I don't wait, I just connect to
>>>> their socket and start talking.  They'll reply only when ready.
>>>>
>>> That only applies when you want to talk to a socket, which I often
>>> don’t do.  Most of the time I use the storage daemon, I pass all
>>> --blockdev and --export options through the command line and don’t
>>>   create any socket at all.  When I use the QSD just to export some
>>> block device, I generally don’t need QMP.
>>
>> If you export via NBD, why can't you just connect to NBD socket?
>
> I’m not sure what exactly you mean by this, because the socket doesn’t
> exist before the QSD is launched.  If I launch the QSD in the
> background and have it create an NBD server on a Unix socket, then
> this socket will not exist until the respective --nbd-server option is
> parsed.  Trying to connect to it immediately after the QSD has been
> launched may work (if the QSD was quicker to parse the option and
> create the server than me trying to connect) or may yield ECONNREFUSED
> or ENOENT, depending on whether the socket file existed before or not.

This is similar to "with common daemons, [...] I just connect to their
socket and start talking."

> Also, outside of the iotests, I personally generally usually use FUSE
> exports instead of NBD exports.

You could wait for the mount to appear with stat -f.

Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

On 24.01.22 10:23, Markus Armbruster wrote:
> Hanna Reitz <hreitz@redhat.com> writes:
>
>> On 21.01.22 15:26, Markus Armbruster wrote:
>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>
>>>> On 21.01.22 11:27, Markus Armbruster wrote:
>>>>> Hanna Reitz <hreitz@redhat.com> writes:
>>>>>> The problem I face is that currently there is no ergonomic way to wait
>>>>>> until the QSD is up and running (besides looping until the PID file
>>>>>> exists), and I don’t think a utility program that doesn’t know the QSD
>>>>>> could provide this.  (For example, it looks like daemonize(1) will
>>>>>> have the parent exit immediately, regardless of whether the child is
>>>>>> set up or not.)
>>>>> Why do you need to wait for QSD to be ready?
>>>>>
>>>>> I'm asking because with common daemons, I don't wait, I just connect to
>>>>> their socket and start talking.  They'll reply only when ready.
>>>>>
>>>> That only applies when you want to talk to a socket, which I often
>>>> don’t do.  Most of the time I use the storage daemon, I pass all
>>>> --blockdev and --export options through the command line and don’t
>>>>    create any socket at all.  When I use the QSD just to export some
>>>> block device, I generally don’t need QMP.
>>> If you export via NBD, why can't you just connect to NBD socket?
>> I’m not sure what exactly you mean by this, because the socket doesn’t
>> exist before the QSD is launched.  If I launch the QSD in the
>> background and have it create an NBD server on a Unix socket, then
>> this socket will not exist until the respective --nbd-server option is
>> parsed.  Trying to connect to it immediately after the QSD has been
>> launched may work (if the QSD was quicker to parse the option and
>> create the server than me trying to connect) or may yield ECONNREFUSED
>> or ENOENT, depending on whether the socket file existed before or not.
> This is similar to "with common daemons, [...] I just connect to their
> socket and start talking."
>
>> Also, outside of the iotests, I personally generally usually use FUSE
>> exports instead of NBD exports.
> You could wait for the mount to appear with stat -f.

As I’ve said from my very first reply on this thread, I could also 
simply use --pidfile and wait for the PID file to appear.

I simply thought “Oh, that doesn’t feel nice, it would be very nice if I 
could simply have an option for QSD to launch it such that it would put 
itself in the background once its done.”