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

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

[Hanna Reitz]

Hi,

v1 cover letter:

https://lists.nongnu.org/archive/html/qemu-block/2021-12/msg00499.html


In v2, I followed Vladimir’s suggestion to look into whether we could
reuse os_daemonize().  Indeed we can, and it makes patch 3 (formerly 2)
much simpler!

I decided to leave patch 2 (formerly 1) largely unchanged, because it
seems to me like the point of contention is whether it’s at all
reasonable to introduce a second argument pass for this feature, and not
e.g. which arguments we parse during it.
I believe such an additional pass is a necessity for --daemonize, so
either we really don’t want this pass and so cannot add this feature
(and just drop this series); or we do want this feature, and then we
have to add this pass.


v2:
- Patch 1: Added, so we can use os_daemonize() in patch 3
  (os_daemonize() internally will only do something if the static
  `daemonize` variable is set, which this new os_set_daemonize()
  function does; otherwise, you can only set it by invoking
  os_parse_cmd_args(), which I would rather not (feels like abuse))

- Patch 2:
  - Tried to be more verbose in the commit description
  - Made it clear in process_options() that only QEMU-specific options
    are processed in order

- Patch 3: Vastly simplified by using the existing os_daemonize() and
  os_setup_post() functions


git backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[down] 'os-posix: Add os_set_daemonize()'
002/4:[0006] [FC] 'qsd: Add pre-init argument parsing pass'
003/4:[0148] [FC] 'qsd: Add --daemonize'
004/4:[----] [--] 'iotests/185: Add post-READY quit tests'


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

 docs/tools/qemu-storage-daemon.rst   |   7 +
 include/sysemu/os-posix.h            |   1 +
 include/sysemu/os-win32.h            |   5 +
 os-posix.c                           |   6 +
 storage-daemon/qemu-storage-daemon.c |  58 +++++++-
 tests/qemu-iotests/185               | 190 ++++++++++++++++++++++++++-
 tests/qemu-iotests/185.out           |  48 +++++++
 7 files changed, 309 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] os-posix: Add os_set_daemonize()

[Hanna Reitz]

The daemonizing functions in os-posix (os_daemonize() and
os_setup_post()) only daemonize the process if the static `daemonize`
variable is set.  Right now, it can only be set by os_parse_cmd_args().

In order to use os_daemonize() and os_setup_post() from the storage
daemon to have it be daemonized, we need some other way to set this
`daemonize` variable, because I would rather not tap into the system
emulator's arg-parsing code.  Therefore, this patch adds an
os_set_daemonize() function, which will return an error on os-win32
(because daemonizing is not supported there).

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 include/sysemu/os-posix.h | 1 +
 include/sysemu/os-win32.h | 5 +++++
 os-posix.c                | 6 ++++++
 3 files changed, 12 insertions(+)

diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 2edf33658a..dd64fb401d 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -55,6 +55,7 @@ int os_mlock(void);
 typedef struct timeval qemu_timeval;
 #define qemu_gettimeofday(tp) gettimeofday(tp, NULL)
 
+int os_set_daemonize(bool d);
 bool is_daemonized(void);
 
 /**
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 43f569b5c2..68af96907e 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -77,6 +77,11 @@ typedef struct {
 } qemu_timeval;
 int qemu_gettimeofday(qemu_timeval *tp);
 
+static inline int os_set_daemonize(bool d)
+{
+    return -ENOTSUP;
+}
+
 static inline bool is_daemonized(void)
 {
     return false;
diff --git a/os-posix.c b/os-posix.c
index ae6c9f2a5e..24692c8593 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -317,6 +317,12 @@ bool is_daemonized(void)
     return daemonize;
 }
 
+int os_set_daemonize(bool d)
+{
+    daemonize = d;
+    return 0;
+}
+
 int os_mlock(void)
 {
 #ifdef HAVE_MLOCKALL
-- 
2.34.1

[PATCH v2 2/4] qsd: Add pre-init argument parsing pass

[Hanna Reitz]

In contrast to qemu-nbd (where it is called --fork) and the system
emulator, QSD does not have a --daemonize switch yet.  Just like them,
QSD allows setting up block devices and exports on the command line.
When doing so, it is often necessary for whoever invoked the QSD to wait
until these exports are fully set up.  A --daemonize switch allows
precisely this, by virtue of the parent process exiting once everything
is set up.

Note that there are alternative ways of waiting for all exports to be
set up, for example:
- Passing the --pidfile option and waiting until the respective file
  exists (but I do not know if there is a way of implementing this
  without a busy wait loop)
- Set up some network server (e.g. on a Unix socket) and have the QSD
  connect to it after all arguments have been processed by appending
  corresponding --chardev and --monitor options to the command line,
  and then wait until the QSD connects

Having a --daemonize option would make this simpler, though, without
having to rely on additional tools (to set up a network server) or busy
waiting.

Implementing a --daemonize switch means having to fork the QSD process.
Ideally, we should do this as early as possible: All the parent process
has to do is to wait for the child process to signal completion of its
set-up phase, and therefore there is basically no initialization that
needs to be done before the fork.  On the other hand, forking after
initialization steps means having to consider how those steps (like
setting up the block layer or QMP) interact with a later fork, which is
often not trivial.

In order to fork this early, we must scan the command line for
--daemonize 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.)

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 | 43 ++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 504d33aa91..b798954edb 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -177,7 +177,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;
 
@@ -196,11 +212,26 @@ static void process_options(int argc, char *argv[])
     };
 
     /*
-     * In contrast to the system emulator, options are processed in the order
-     * they are given on the command lines. This means that things must be
-     * defined first before they can be referenced in another option.
+     * In contrast to the system emulator, QEMU-specific options are processed
+     * in the order 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);
@@ -334,6 +365,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);
@@ -348,7 +381,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.34.1

[PATCH v2 3/4] qsd: Add --daemonize

[Hanna Reitz]

To implement this, we reuse the existing daemonizing functions from the
system emulator, which mainly do the following:
- Fork off a child process, and set up a pipe between parent and child
- The parent process waits until the child sends a status byte over the
  pipe (0 means that the child was set up successfully; anything else
  (including errors or EOF) means that the child was not set up
  successfully), and then exits with an appropriate exit status
- The child process enters a new session (forking off again), changes
  the umask, and will ignore terminal signals from then on
- Once set-up is complete, the child will chdir to /, redirect all
  standard I/O streams to /dev/null, and tell the parent that set-up has
  been completed successfully

In contrast to qemu-nbd's --fork implementation, during the set up
phase, error messages are not piped through the parent process.
qemu-nbd mainly does this to detect errors, though (while os_daemonize()
has the child explicitly signal success after set up); because we do not
redirect stderr after forking, error messages continue to appear on
whatever the parent's stderr was (until set up is complete).

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

diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
index 878e6a5c5c..8b97592663 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -154,6 +154,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 b798954edb..9f2c3332bf 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -137,6 +137,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());
 }
@@ -144,6 +147,7 @@ QEMU_HELP_BOTTOM "\n",
 enum {
     OPTION_BLOCKDEV = 256,
     OPTION_CHARDEV,
+    OPTION_DAEMONIZE,
     OPTION_EXPORT,
     OPTION_MONITOR,
     OPTION_NBD_SERVER,
@@ -200,6 +204,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},
@@ -225,6 +230,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 */
@@ -277,6 +283,12 @@ static void process_options(int argc, char *argv[], bool pre_init_pass)
                 qemu_opts_del(opts);
                 break;
             }
+        case OPTION_DAEMONIZE:
+            if (os_set_daemonize(true) < 0) {
+                error_report("--daemonize not supported in this build");
+                exit(EXIT_FAILURE);
+            }
+            break;
         case OPTION_EXPORT:
             {
                 Visitor *v;
@@ -367,6 +379,8 @@ int main(int argc, char *argv[])
 
     process_options(argc, argv, true);
 
+    os_daemonize();
+
     module_call_init(MODULE_INIT_QOM);
     module_call_init(MODULE_INIT_TRACE);
     qemu_add_opts(&qemu_trace_opts);
@@ -389,6 +403,7 @@ int main(int argc, char *argv[])
      * it.
      */
     pid_file_init();
+    os_setup_post();
 
     while (!exit_requested) {
         main_loop_wait(false);
-- 
2.34.1

[PATCH v2 4/4] 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.34.1

Re: [PATCH v2 1/4] os-posix: Add os_set_daemonize()

[Eric Blake]

On Thu, Mar 03, 2022 at 05:48:11PM +0100, Hanna Reitz wrote:
> The daemonizing functions in os-posix (os_daemonize() and
> os_setup_post()) only daemonize the process if the static `daemonize`
> variable is set.  Right now, it can only be set by os_parse_cmd_args().
> 
> In order to use os_daemonize() and os_setup_post() from the storage
> daemon to have it be daemonized, we need some other way to set this
> `daemonize` variable, because I would rather not tap into the system
> emulator's arg-parsing code.  Therefore, this patch adds an
> os_set_daemonize() function, which will return an error on os-win32
> (because daemonizing is not supported there).
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---

> +++ b/include/sysemu/os-win32.h
> @@ -77,6 +77,11 @@ typedef struct {
>  } qemu_timeval;
>  int qemu_gettimeofday(qemu_timeval *tp);
>  
> +static inline int os_set_daemonize(bool d)
> +{
> +    return -ENOTSUP;

Should this fail only if d is true?  Or will all callers only ever
pass true, in which case why do we need the paraemeter?

> +}
> +
>  static inline bool is_daemonized(void)
>  {
>      return false;
> diff --git a/os-posix.c b/os-posix.c
> index ae6c9f2a5e..24692c8593 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -317,6 +317,12 @@ bool is_daemonized(void)
>      return daemonize;
>  }
>  
> +int os_set_daemonize(bool d)
> +{
> +    daemonize = d;
> +    return 0;
> +}
> +
>  int os_mlock(void)
>  {
>  #ifdef HAVE_MLOCKALL
> -- 
> 2.34.1
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/4] qsd: Add pre-init argument parsing pass

[Eric Blake]

On Thu, Mar 03, 2022 at 05:48:12PM +0100, Hanna Reitz wrote:
> In contrast to qemu-nbd (where it is called --fork) and the system
> emulator, QSD does not have a --daemonize switch yet.  Just like them,
> QSD allows setting up block devices and exports on the command line.
> When doing so, it is often necessary for whoever invoked the QSD to wait
> until these exports are fully set up.  A --daemonize switch allows
> precisely this, by virtue of the parent process exiting once everything
> is set up.
> 
> Note that there are alternative ways of waiting for all exports to be
> set up, for example:
> - Passing the --pidfile option and waiting until the respective file
>   exists (but I do not know if there is a way of implementing this
>   without a busy wait loop)

Non-portably, you could use inotify or similar, to get a true
event-driven wakeup when the file is created.  And here's the python
glue that libnbd uses, instead of --pidfile:

https://gitlab.com/nbdkit/libnbd/-/blob/master/interop/interop-qemu-storage-daemon.sh#L58

> - Set up some network server (e.g. on a Unix socket) and have the QSD
>   connect to it after all arguments have been processed by appending
>   corresponding --chardev and --monitor options to the command line,
>   and then wait until the QSD connects
> 
> Having a --daemonize option would make this simpler, though, without
> having to rely on additional tools (to set up a network server) or busy
> waiting.
> 
> Implementing a --daemonize switch means having to fork the QSD process.
> Ideally, we should do this as early as possible: All the parent process
> has to do is to wait for the child process to signal completion of its
> set-up phase, and therefore there is basically no initialization that
> needs to be done before the fork.  On the other hand, forking after
> initialization steps means having to consider how those steps (like
> setting up the block layer or QMP) interact with a later fork, which is
> often not trivial.
> 
> In order to fork this early, we must scan the command line for
> --daemonize 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

s/but/beyond/

> 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.

Well, GNU ls does that, but only if POSIXLY_CORRECT is not set (a
strict POSIX ls must give you two 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.)
> 
> 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 | 43 ++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 5 deletions(-)
>

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 3/4] qsd: Add --daemonize

[Eric Blake]

On Thu, Mar 03, 2022 at 05:48:13PM +0100, Hanna Reitz wrote:
> To implement this, we reuse the existing daemonizing functions from the
> system emulator, which mainly do the following:
> - Fork off a child process, and set up a pipe between parent and child
> - The parent process waits until the child sends a status byte over the
>   pipe (0 means that the child was set up successfully; anything else
>   (including errors or EOF) means that the child was not set up
>   successfully), and then exits with an appropriate exit status
> - The child process enters a new session (forking off again), changes
>   the umask, and will ignore terminal signals from then on
> - Once set-up is complete, the child will chdir to /, redirect all
>   standard I/O streams to /dev/null, and tell the parent that set-up has
>   been completed successfully
> 
> In contrast to qemu-nbd's --fork implementation, during the set up
> phase, error messages are not piped through the parent process.
> qemu-nbd mainly does this to detect errors, though (while os_daemonize()
> has the child explicitly signal success after set up); because we do not
> redirect stderr after forking, error messages continue to appear on
> whatever the parent's stderr was (until set up is complete).
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  docs/tools/qemu-storage-daemon.rst   |  7 +++++++
>  storage-daemon/qemu-storage-daemon.c | 15 +++++++++++++++
>  2 files changed, 22 insertions(+)
>

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 4/4] iotests/185: Add post-READY quit tests

[Eric Blake]

On Thu, Mar 03, 2022 at 05:48:14PM +0100, Hanna Reitz wrote:
> 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))

I tend to write $((64 * 1024 * 1024)) rather than remembering all the
digits of 2^20, but your way is fine.

Nice test addition!

Reviewed-by: Eric Blake <eblake@redhat.com>

I'm happy to queue this series through my NBD tree in time for
softfreeze, if no one else speaks for it first.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 1/4] os-posix: Add os_set_daemonize()

[Daniel P. Berrangé]

On Thu, Mar 03, 2022 at 05:48:11PM +0100, Hanna Reitz wrote:
> The daemonizing functions in os-posix (os_daemonize() and
> os_setup_post()) only daemonize the process if the static `daemonize`
> variable is set.  Right now, it can only be set by os_parse_cmd_args().
> 
> In order to use os_daemonize() and os_setup_post() from the storage
> daemon to have it be daemonized, we need some other way to set this
> `daemonize` variable, because I would rather not tap into the system
> emulator's arg-parsing code.  Therefore, this patch adds an
> os_set_daemonize() function, which will return an error on os-win32
> (because daemonizing is not supported there).

IMHO the real flaw here is the design of 'os_daemonize' in that it
relies on static state. If I see a call to a function 'os_daemonize()'
I expect to be daemonized on return, but with this design that is not
guaranteed which is a big surprise.

I'd suggest we push the condition into the caller instead of adding
this extra function, so we have the more sane pattern:

   if (daemonmize()) {
      os_daemonize()
   }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 3/4] qsd: Add --daemonize

[Kevin Wolf]

Am 03.03.2022 um 17:48 hat Hanna Reitz geschrieben:
> To implement this, we reuse the existing daemonizing functions from the
> system emulator, which mainly do the following:
> - Fork off a child process, and set up a pipe between parent and child
> - The parent process waits until the child sends a status byte over the
>   pipe (0 means that the child was set up successfully; anything else
>   (including errors or EOF) means that the child was not set up
>   successfully), and then exits with an appropriate exit status
> - The child process enters a new session (forking off again), changes
>   the umask, and will ignore terminal signals from then on
> - Once set-up is complete, the child will chdir to /, redirect all
>   standard I/O streams to /dev/null, and tell the parent that set-up has
>   been completed successfully
> 
> In contrast to qemu-nbd's --fork implementation, during the set up
> phase, error messages are not piped through the parent process.
> qemu-nbd mainly does this to detect errors, though (while os_daemonize()
> has the child explicitly signal success after set up); because we do not
> redirect stderr after forking, error messages continue to appear on
> whatever the parent's stderr was (until set up is complete).
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  docs/tools/qemu-storage-daemon.rst   |  7 +++++++
>  storage-daemon/qemu-storage-daemon.c | 15 +++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> index 878e6a5c5c..8b97592663 100644
> --- a/docs/tools/qemu-storage-daemon.rst
> +++ b/docs/tools/qemu-storage-daemon.rst
> @@ -154,6 +154,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 b798954edb..9f2c3332bf 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -137,6 +137,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"

So far the long options in the help text are sorted alphabetically. Do
we want to keep this?

Kevin

Re: [PATCH v2 1/4] os-posix: Add os_set_daemonize()

[Kevin Wolf]

Am 04.03.2022 um 10:19 hat Daniel P. Berrangé geschrieben:
> On Thu, Mar 03, 2022 at 05:48:11PM +0100, Hanna Reitz wrote:
> > The daemonizing functions in os-posix (os_daemonize() and
> > os_setup_post()) only daemonize the process if the static `daemonize`
> > variable is set.  Right now, it can only be set by os_parse_cmd_args().
> > 
> > In order to use os_daemonize() and os_setup_post() from the storage
> > daemon to have it be daemonized, we need some other way to set this
> > `daemonize` variable, because I would rather not tap into the system
> > emulator's arg-parsing code.  Therefore, this patch adds an
> > os_set_daemonize() function, which will return an error on os-win32
> > (because daemonizing is not supported there).
> 
> IMHO the real flaw here is the design of 'os_daemonize' in that it
> relies on static state. If I see a call to a function 'os_daemonize()'
> I expect to be daemonized on return, but with this design that is not
> guaranteed which is a big surprise.
> 
> I'd suggest we push the condition into the caller instead of adding
> this extra function, so we have the more sane pattern:
> 
>    if (daemonmize()) {
>       os_daemonize()
>    }

It's not as simple, the static daemonize variable is used in more places
than just os_daemonize(). I'm not sure if it's worth changing how all of
this works, but if we did, it would be a refactoring mostly focussed on
the system emulator and an issue separate from adding the option to the
storage daemon.

Kevin

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

[Kevin Wolf]

Am 03.03.2022 um 17:48 hat Hanna Reitz geschrieben:
> Hi,
> 
> v1 cover letter:
> 
> https://lists.nongnu.org/archive/html/qemu-block/2021-12/msg00499.html
> 
> 
> In v2, I followed Vladimir’s suggestion to look into whether we could
> reuse os_daemonize().  Indeed we can, and it makes patch 3 (formerly 2)
> much simpler!
> 
> I decided to leave patch 2 (formerly 1) largely unchanged, because it
> seems to me like the point of contention is whether it’s at all
> reasonable to introduce a second argument pass for this feature, and not
> e.g. which arguments we parse during it.
> I believe such an additional pass is a necessity for --daemonize, so
> either we really don’t want this pass and so cannot add this feature
> (and just drop this series); or we do want this feature, and then we
> have to add this pass.

Thanks, fixed up as discussed on IRC to address the two minor comments
from Eric and myself, and applied to the block branch.

Kevin

Re: [PATCH v2 1/4] os-posix: Add os_set_daemonize()

[Daniel P. Berrangé]

On Fri, Mar 04, 2022 at 11:20:39AM +0100, Kevin Wolf wrote:
> Am 04.03.2022 um 10:19 hat Daniel P. Berrangé geschrieben:
> > On Thu, Mar 03, 2022 at 05:48:11PM +0100, Hanna Reitz wrote:
> > > The daemonizing functions in os-posix (os_daemonize() and
> > > os_setup_post()) only daemonize the process if the static `daemonize`
> > > variable is set.  Right now, it can only be set by os_parse_cmd_args().
> > > 
> > > In order to use os_daemonize() and os_setup_post() from the storage
> > > daemon to have it be daemonized, we need some other way to set this
> > > `daemonize` variable, because I would rather not tap into the system
> > > emulator's arg-parsing code.  Therefore, this patch adds an
> > > os_set_daemonize() function, which will return an error on os-win32
> > > (because daemonizing is not supported there).
> > 
> > IMHO the real flaw here is the design of 'os_daemonize' in that it
> > relies on static state. If I see a call to a function 'os_daemonize()'
> > I expect to be daemonized on return, but with this design that is not
> > guaranteed which is a big surprise.
> > 
> > I'd suggest we push the condition into the caller instead of adding
> > this extra function, so we have the more sane pattern:
> > 
> >    if (daemonmize()) {
> >       os_daemonize()
> >    }
> 
> It's not as simple, the static daemonize variable is used in more places
> than just os_daemonize(). I'm not sure if it's worth changing how all of
> this works, but if we did, it would be a refactoring mostly focussed on
> the system emulator and an issue separate from adding the option to the
> storage daemon.

It isn't that difficult to do the refactoring needed, so I've just
sent a series that does the job and CC folks from this thread on it.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|