[PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output

[PATCH v3 0/8] qemu-nbd: Restore "qemu-nbd -v --fork" output

[Denis V. Lunev]

This series contains the fix to the regression introduced with 5c56dd27a2
plus documentation update.

Changes from v2:
- fixed error message of dup2 (patch #1)
- removed options copy as suggested by Eric
- moved all other options used in nbd_client_thread() to struct NbdClientOpts
  for consistency
- added documentation update to document -v behavior
- small formatting fix (please drop if this is extra)

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[PATCH 1/8] qemu-nbd: improve error message for dup2 error

[Denis V. Lunev]

This error is happened when we are not able to close the pipe to the
parent (to trace errors in the child process) and assign stderr to
/dev/null as required by the daemonizing convention.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Suggested-by: Eric Blake <eblake@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qemu-nbd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index aaccaa3318..4575e4291e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -324,7 +324,7 @@ static void *nbd_client_thread(void *arg)
     } else {
         /* Close stderr so that the qemu-nbd process exits.  */
         if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
-            error_report("Could not set stderr to /dev/null: %s",
+            error_report("Could not release pipe to parent: %s",
                          strerror(errno));
             exit(EXIT_FAILURE);
         }
@@ -1181,7 +1181,7 @@ int main(int argc, char **argv)
 
     if (fork_process) {
         if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
-            error_report("Could not set stderr to /dev/null: %s",
+            error_report("Could not release pipe to parent: %s",
                          strerror(errno));
             exit(EXIT_FAILURE);
         }
-- 
2.34.1

[PATCH 2/8] qemu-nbd: define struct NbdClientOpts when HAVE_NBD_DEVICE is not defined

[Denis V. Lunev]

This patch also drops definition of some locals in main() to avoid
useless data copy.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qemu-nbd.c | 60 ++++++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4575e4291e..ebfae4d0b6 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -253,6 +253,12 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
 }
 
 
+struct NbdClientOpts {
+    char *device;
+    bool fork_process;
+    bool verbose;
+};
+
 #if HAVE_NBD_DEVICE
 static void *show_parts(void *arg)
 {
@@ -271,12 +277,6 @@ static void *show_parts(void *arg)
     return NULL;
 }
 
-struct NbdClientOpts {
-    char *device;
-    bool fork_process;
-    bool verbose;
-};
-
 static void *nbd_client_thread(void *arg)
 {
     struct NbdClientOpts *opts = arg;
@@ -519,7 +519,6 @@ int main(int argc, char **argv)
     const char *bindto = NULL;
     const char *port = NULL;
     char *sockpath = NULL;
-    char *device = NULL;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
     const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:AB:L";
@@ -582,16 +581,16 @@ int main(int argc, char **argv)
     const char *tlshostname = NULL;
     bool imageOpts = false;
     bool writethrough = false; /* Client will flush as needed. */
-    bool verbose = false;
-    bool fork_process = false;
     bool list = false;
     unsigned socket_activation;
     const char *pid_file_name = NULL;
     const char *selinux_label = NULL;
     BlockExportOptions *export_opts;
-#if HAVE_NBD_DEVICE
-    struct NbdClientOpts opts;
-#endif
+    struct NbdClientOpts opts = {
+        .fork_process = false,
+        .verbose = false,
+        .device = NULL,
+    };
 
 #ifdef CONFIG_POSIX
     os_setup_early_signal_handling();
@@ -719,7 +718,7 @@ int main(int argc, char **argv)
             disconnect = true;
             break;
         case 'c':
-            device = optarg;
+            opts.device = optarg;
             break;
         case 'e':
             if (qemu_strtoi(optarg, NULL, 0, &shared) < 0 ||
@@ -750,7 +749,7 @@ int main(int argc, char **argv)
             }
             break;
         case 'v':
-            verbose = true;
+            opts.verbose = true;
             break;
         case 'V':
             version(argv[0]);
@@ -782,7 +781,7 @@ int main(int argc, char **argv)
             tlsauthz = optarg;
             break;
         case QEMU_NBD_OPT_FORK:
-            fork_process = true;
+            opts.fork_process = true;
             break;
         case 'L':
             list = true;
@@ -802,12 +801,12 @@ int main(int argc, char **argv)
             exit(EXIT_FAILURE);
         }
         if (export_name || export_description || dev_offset ||
-            device || disconnect || fmt || sn_id_or_name || bitmaps ||
+            opts.device || disconnect || fmt || sn_id_or_name || bitmaps ||
             alloc_depth || seen_aio || seen_discard || seen_cache) {
             error_report("List mode is incompatible with per-device settings");
             exit(EXIT_FAILURE);
         }
-        if (fork_process) {
+        if (opts.fork_process) {
             error_report("List mode is incompatible with forking");
             exit(EXIT_FAILURE);
         }
@@ -832,7 +831,8 @@ int main(int argc, char **argv)
         }
     } else {
         /* Using socket activation - check user didn't use -p etc. */
-        const char *err_msg = socket_activation_validate_opts(device, sockpath,
+        const char *err_msg = socket_activation_validate_opts(opts.device,
+                                                              sockpath,
                                                               bindto, port,
                                                               selinux_label,
                                                               list);
@@ -850,7 +850,7 @@ int main(int argc, char **argv)
     }
 
     if (tlscredsid) {
-        if (device) {
+        if (opts.device) {
             error_report("TLS is not supported with a host device");
             exit(EXIT_FAILURE);
         }
@@ -880,7 +880,7 @@ int main(int argc, char **argv)
 
     if (selinux_label) {
 #ifdef CONFIG_SELINUX
-        if (sockpath == NULL && device == NULL) {
+        if (sockpath == NULL && opts.device == NULL) {
             error_report("--selinux-label is not permitted without --socket");
             exit(EXIT_FAILURE);
         }
@@ -897,7 +897,7 @@ int main(int argc, char **argv)
     }
 
 #if !HAVE_NBD_DEVICE
-    if (disconnect || device) {
+    if (disconnect || opts.device) {
         error_report("Kernel /dev/nbdN support not available");
         exit(EXIT_FAILURE);
     }
@@ -919,7 +919,7 @@ int main(int argc, char **argv)
     }
 #endif
 
-    if ((device && !verbose) || fork_process) {
+    if ((opts.device && !opts.verbose) || opts.fork_process) {
 #ifndef WIN32
         g_autoptr(GError) err = NULL;
         int stderr_fd[2];
@@ -1002,9 +1002,9 @@ int main(int argc, char **argv)
 #endif /* WIN32 */
     }
 
-    if (device != NULL && sockpath == NULL) {
+    if (opts.device != NULL && sockpath == NULL) {
         sockpath = g_malloc(128);
-        snprintf(sockpath, 128, SOCKET_PATH, basename(device));
+        snprintf(sockpath, 128, SOCKET_PATH, basename(opts.device));
     }
 
     server = qio_net_listener_new();
@@ -1145,15 +1145,9 @@ int main(int argc, char **argv)
     blk_exp_add(export_opts, &error_fatal);
     qapi_free_BlockExportOptions(export_opts);
 
-    if (device) {
+    if (opts.device) {
 #if HAVE_NBD_DEVICE
         int ret;
-        opts = (struct NbdClientOpts) {
-            .device = device,
-            .fork_process = fork_process,
-            .verbose = verbose,
-        };
-
         ret = pthread_create(&client_thread, NULL, nbd_client_thread, &opts);
         if (ret != 0) {
             error_report("Failed to create client thread: %s", strerror(ret));
@@ -1179,7 +1173,7 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
-    if (fork_process) {
+    if (opts.fork_process) {
         if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
             error_report("Could not release pipe to parent: %s",
                          strerror(errno));
@@ -1203,7 +1197,7 @@ int main(int argc, char **argv)
 
     qemu_opts_del(sn_opts);
 
-    if (device) {
+    if (opts.device) {
         void *ret;
         pthread_join(client_thread, &ret);
         exit(ret != NULL);
-- 
2.34.1

[PATCH 3/8] qemu-nbd: move srcpath into struct NbdClientOpts

[Denis V. Lunev]

We pass other parameters into nbd_client_thread() in this way. This patch
makes the code more consistent.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qemu-nbd.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index ebfae4d0b6..de6c2be590 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -73,7 +73,6 @@
 
 #define MBR_SIZE 512
 
-static char *srcpath;
 static SocketAddress *saddr;
 static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATED } state;
@@ -255,6 +254,7 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
 
 struct NbdClientOpts {
     char *device;
+    char *srcpath;
     bool fork_process;
     bool verbose;
 };
@@ -320,7 +320,7 @@ static void *nbd_client_thread(void *arg)
 
     if (opts->verbose && !opts->fork_process) {
         fprintf(stderr, "NBD device %s is now connected to %s\n",
-                opts->device, srcpath);
+                opts->device, opts->srcpath);
     } else {
         /* Close stderr so that the qemu-nbd process exits.  */
         if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
@@ -590,6 +590,7 @@ int main(int argc, char **argv)
         .fork_process = false,
         .verbose = false,
         .device = NULL,
+        .srcpath = NULL,
     };
 
 #ifdef CONFIG_POSIX
@@ -1059,19 +1060,19 @@ int main(int argc, char **argv)
     bdrv_init();
     atexit(qemu_nbd_shutdown);
 
-    srcpath = argv[optind];
+    opts.srcpath = argv[optind];
     if (imageOpts) {
-        QemuOpts *opts;
+        QemuOpts *o;
         if (fmt) {
             error_report("--image-opts and -f are mutually exclusive");
             exit(EXIT_FAILURE);
         }
-        opts = qemu_opts_parse_noisily(&file_opts, srcpath, true);
-        if (!opts) {
+        o = qemu_opts_parse_noisily(&file_opts, opts.srcpath, true);
+        if (!o) {
             qemu_opts_reset(&file_opts);
             exit(EXIT_FAILURE);
         }
-        options = qemu_opts_to_qdict(opts, NULL);
+        options = qemu_opts_to_qdict(o, NULL);
         qemu_opts_reset(&file_opts);
         blk = blk_new_open(NULL, NULL, options, flags, &local_err);
     } else {
@@ -1079,7 +1080,7 @@ int main(int argc, char **argv)
             options = qdict_new();
             qdict_put_str(options, "driver", fmt);
         }
-        blk = blk_new_open(srcpath, NULL, options, flags, &local_err);
+        blk = blk_new_open(opts.srcpath, NULL, options, flags, &local_err);
     }
 
     if (!blk) {
-- 
2.34.1

[PATCH 4/8] qemu-nbd: put saddr into into struct NbdClientOpts

[Denis V. Lunev]

We pass other parameters into nbd_client_thread() in this way. This patch
makes the code more consistent.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qemu-nbd.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index de6c2be590..d0f8d8bad2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -73,7 +73,6 @@
 
 #define MBR_SIZE 512
 
-static SocketAddress *saddr;
 static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATED } state;
 static int shared = 1;
@@ -255,6 +254,7 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
 struct NbdClientOpts {
     char *device;
     char *srcpath;
+    SocketAddress *saddr;
     bool fork_process;
     bool verbose;
 };
@@ -289,7 +289,7 @@ static void *nbd_client_thread(void *arg)
 
     sioc = qio_channel_socket_new();
     if (qio_channel_socket_connect_sync(sioc,
-                                        saddr,
+                                        opts->saddr,
                                         &local_error) < 0) {
         error_report_err(local_error);
         goto out;
@@ -591,6 +591,7 @@ int main(int argc, char **argv)
         .verbose = false,
         .device = NULL,
         .srcpath = NULL,
+        .saddr = NULL,
     };
 
 #ifdef CONFIG_POSIX
@@ -892,8 +893,8 @@ int main(int argc, char **argv)
     }
 
     if (list) {
-        saddr = nbd_build_socket_address(sockpath, bindto, port);
-        return qemu_nbd_client_list(saddr, tlscreds,
+        opts.saddr = nbd_build_socket_address(sockpath, bindto, port);
+        return qemu_nbd_client_list(opts.saddr, tlscreds,
                                     tlshostname ? tlshostname : bindto);
     }
 
@@ -1024,8 +1025,8 @@ int main(int argc, char **argv)
             exit(EXIT_FAILURE);
         }
 #endif
-        saddr = nbd_build_socket_address(sockpath, bindto, port);
-        if (qio_net_listener_open_sync(server, saddr, backlog,
+        opts.saddr = nbd_build_socket_address(sockpath, bindto, port);
+        if (qio_net_listener_open_sync(server, opts.saddr, backlog,
                                        &local_err) < 0) {
             object_unref(OBJECT(server));
             error_report_err(local_err);
-- 
2.34.1

[PATCH 5/8] qemu-nbd: invent nbd_client_release_pipe() helper

[Denis V. Lunev]

Move the code from main() and nbd_client_thread() into the specific
helper. This code is going to be grown.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qemu-nbd.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index d0f8d8bad2..9f28e3ebda 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -259,6 +259,16 @@ struct NbdClientOpts {
     bool verbose;
 };
 
+static void nbd_client_release_pipe(void)
+{
+    /* Close stderr so that the qemu-nbd process exits.  */
+    if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
+        error_report("Could not release pipe to parent: %s",
+                     strerror(errno));
+        exit(EXIT_FAILURE);
+    }
+}
+
 #if HAVE_NBD_DEVICE
 static void *show_parts(void *arg)
 {
@@ -322,12 +332,7 @@ static void *nbd_client_thread(void *arg)
         fprintf(stderr, "NBD device %s is now connected to %s\n",
                 opts->device, opts->srcpath);
     } else {
-        /* Close stderr so that the qemu-nbd process exits.  */
-        if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
-            error_report("Could not release pipe to parent: %s",
-                         strerror(errno));
-            exit(EXIT_FAILURE);
-        }
+        nbd_client_release_pipe();
     }
 
     if (nbd_client(fd) < 0) {
@@ -1176,11 +1181,7 @@ int main(int argc, char **argv)
     }
 
     if (opts.fork_process) {
-        if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
-            error_report("Could not release pipe to parent: %s",
-                         strerror(errno));
-            exit(EXIT_FAILURE);
-        }
+        nbd_client_release_pipe();
     }
 
     state = RUNNING;
-- 
2.34.1

[PATCH 6/8] qemu-nbd: Restore "qemu-nbd -v --fork" output

[Denis V. Lunev]

Closing stderr earlier is good for daemonized qemu-nbd under ssh
earlier, but breaks the case where -v is being used to track what is
happening in the server, as in iotest 233.

When we know we are verbose, we should preserve original stderr and
restore it once the setup stage is done. This commit restores the
original behavior with -v option. In this case original output
inside the test is kept intact.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Mike Maslenkin <mike.maslenkin@gmail.com>
Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh")
---
 qemu-nbd.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9f28e3ebda..b9c74ce77c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -255,18 +255,23 @@ struct NbdClientOpts {
     char *device;
     char *srcpath;
     SocketAddress *saddr;
+    int stderr;
     bool fork_process;
     bool verbose;
 };
 
-static void nbd_client_release_pipe(void)
+static void nbd_client_release_pipe(int old_stderr)
 {
     /* Close stderr so that the qemu-nbd process exits.  */
-    if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
+    if (dup2(old_stderr, STDERR_FILENO) < 0) {
         error_report("Could not release pipe to parent: %s",
                      strerror(errno));
         exit(EXIT_FAILURE);
     }
+    if (old_stderr != STDOUT_FILENO && close(old_stderr) < 0) {
+        error_report("Could not release qemu-nbd: %s", strerror(errno));
+        exit(EXIT_FAILURE);
+    }
 }
 
 #if HAVE_NBD_DEVICE
@@ -332,7 +337,7 @@ static void *nbd_client_thread(void *arg)
         fprintf(stderr, "NBD device %s is now connected to %s\n",
                 opts->device, opts->srcpath);
     } else {
-        nbd_client_release_pipe();
+        nbd_client_release_pipe(opts->stderr);
     }
 
     if (nbd_client(fd) < 0) {
@@ -597,6 +602,7 @@ int main(int argc, char **argv)
         .device = NULL,
         .srcpath = NULL,
         .saddr = NULL,
+        .stderr = STDOUT_FILENO,
     };
 
 #ifdef CONFIG_POSIX
@@ -951,6 +957,16 @@ int main(int argc, char **argv)
 
             close(stderr_fd[0]);
 
+            /* Remember parent's stderr if we will be restoring it. */
+            if (opts.verbose /* fork_process is set */) {
+                opts.stderr = dup(STDERR_FILENO);
+                if (opts.stderr < 0) {
+                    error_report("Could not dup original stderr: %s",
+                                 strerror(errno));
+                    exit(EXIT_FAILURE);
+                }
+            }
+
             ret = qemu_daemon(1, 0);
             saved_errno = errno;    /* dup2 will overwrite error below */
 
@@ -1181,7 +1197,7 @@ int main(int argc, char **argv)
     }
 
     if (opts.fork_process) {
-        nbd_client_release_pipe();
+        nbd_client_release_pipe(opts.stderr);
     }
 
     state = RUNNING;
-- 
2.34.1

[PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 docs/tools/qemu-nbd.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
index faf6349ea5..5c48ee7345 100644
--- a/docs/tools/qemu-nbd.rst
+++ b/docs/tools/qemu-nbd.rst
@@ -197,7 +197,9 @@ driver options if :option:`--image-opts` is specified.
 
 .. option:: -v, --verbose
 
-  Display extra debugging information.
+  Display extra debugging information. This option also keeps opened original
+  *STDERR* stream if ``qemu-nbd`` process is daemonized due to other options
+  like :option:`--fork` or :option:`-c`.
 
 .. option:: -h, --help
 
-- 
2.34.1

[PATCH 8/8] qemu-nbd: fix formatting in main()

[Denis V. Lunev]

Just a formatting, no functional changes.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
Do not really sure that this patch is mandatory, just stabs my eye. Feel free
to drop if this is too useless.

 qemu-nbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b9c74ce77c..8eb1d1f40b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -581,7 +581,8 @@ int main(int argc, char **argv)
     pthread_t client_thread;
     const char *fmt = NULL;
     Error *local_err = NULL;
-    BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
+    BlockdevDetectZeroesOptions detect_zeroes =
+                                            BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
     const char *export_name = NULL; /* defaults to "" later for server mode */
     const char *export_description = NULL;
-- 
2.34.1

Re: [PATCH 1/8] qemu-nbd: improve error message for dup2 error

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:03AM +0200, Denis V. Lunev wrote:
> This error is happened when we are not able to close the pipe to the

s/is happened when/happens if/

> parent (to trace errors in the child process) and assign stderr to
> /dev/null as required by the daemonizing convention.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Suggested-by: Eric Blake <eblake@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  qemu-nbd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index aaccaa3318..4575e4291e 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -324,7 +324,7 @@ static void *nbd_client_thread(void *arg)
>      } else {
>          /* Close stderr so that the qemu-nbd process exits.  */
>          if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
> -            error_report("Could not set stderr to /dev/null: %s",
> +            error_report("Could not release pipe to parent: %s",
>                           strerror(errno));
>              exit(EXIT_FAILURE);
>          }
> @@ -1181,7 +1181,7 @@ int main(int argc, char **argv)
>  
>      if (fork_process) {
>          if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
> -            error_report("Could not set stderr to /dev/null: %s",
> +            error_report("Could not release pipe to parent: %s",
>                           strerror(errno));
>              exit(EXIT_FAILURE);
>          }
> -- 
> 2.34.1
>

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 2/8] qemu-nbd: define struct NbdClientOpts when HAVE_NBD_DEVICE is not defined

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:04AM +0200, Denis V. Lunev wrote:
> This patch also drops definition of some locals in main() to avoid
> useless data copy.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  qemu-nbd.c | 60 ++++++++++++++++++++++++------------------------------
>  1 file changed, 27 insertions(+), 33 deletions(-)

> @@ -519,7 +519,6 @@ int main(int argc, char **argv)
>      const char *bindto = NULL;
>      const char *port = NULL;
>      char *sockpath = NULL;
> -    char *device = NULL;
>      QemuOpts *sn_opts = NULL;
>      const char *sn_id_or_name = NULL;
>      const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:AB:L";
> @@ -582,16 +581,16 @@ int main(int argc, char **argv)
>      const char *tlshostname = NULL;
>      bool imageOpts = false;
>      bool writethrough = false; /* Client will flush as needed. */
> -    bool verbose = false;
> -    bool fork_process = false;
>      bool list = false;
>      unsigned socket_activation;
>      const char *pid_file_name = NULL;
>      const char *selinux_label = NULL;
>      BlockExportOptions *export_opts;
> -#if HAVE_NBD_DEVICE
> -    struct NbdClientOpts opts;
> -#endif
> +    struct NbdClientOpts opts = {
> +        .fork_process = false,
> +        .verbose = false,
> +        .device = NULL,
> +    };

Could also do 'struct NbdClietnOpts opts = {};' since you happen to be
zero-initializing, but this may not remain the case if more fields get
added to the struct, so I'm fine leaving it as written.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 3/8] qemu-nbd: move srcpath into struct NbdClientOpts

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:05AM +0200, Denis V. Lunev wrote:
> We pass other parameters into nbd_client_thread() in this way. This patch
> makes the code more consistent.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  qemu-nbd.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 

> @@ -1059,19 +1060,19 @@ int main(int argc, char **argv)
>      bdrv_init();
>      atexit(qemu_nbd_shutdown);
>  
> -    srcpath = argv[optind];
> +    opts.srcpath = argv[optind];
>      if (imageOpts) {
> -        QemuOpts *opts;
> +        QemuOpts *o;
>          if (fmt) {
>              error_report("--image-opts and -f are mutually exclusive");
>              exit(EXIT_FAILURE);
>          }
> -        opts = qemu_opts_parse_noisily(&file_opts, srcpath, true);
> -        if (!opts) {
> +        o = qemu_opts_parse_noisily(&file_opts, opts.srcpath, true);
> +        if (!o) {

Hmm - this would have been flagged by -Wshadow, and there are other
series working to clean up tree-wide issues that shadowing can cause.
Looking again, the shadowing was previously introduced before this
series, but only when HAVE_NBD_DEVICE was defined; then patch 2/8 made
the shadowing unconditional.  Reworking the series to clean up the
shadowing earlier in 2/8 is just churn, so I don't mind that it took
us to this point to notice it; however, I'm inclined to add a note to
the commit message that it is a (happy) side-effect.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 4/8] qemu-nbd: put saddr into into struct NbdClientOpts

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:06AM +0200, Denis V. Lunev wrote:
> We pass other parameters into nbd_client_thread() in this way. This patch
> makes the code more consistent.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  qemu-nbd.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 5/8] qemu-nbd: invent nbd_client_release_pipe() helper

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:07AM +0200, Denis V. Lunev wrote:
> Move the code from main() and nbd_client_thread() into the specific
> helper. This code is going to be grown.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  qemu-nbd.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 6/8] qemu-nbd: Restore "qemu-nbd -v --fork" output

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:08AM +0200, Denis V. Lunev wrote:
> Closing stderr earlier is good for daemonized qemu-nbd under ssh
> earlier, but breaks the case where -v is being used to track what is
> happening in the server, as in iotest 233.
> 
> When we know we are verbose, we should preserve original stderr and
> restore it once the setup stage is done. This commit restores the
> original behavior with -v option. In this case original output
> inside the test is kept intact.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Mike Maslenkin <mike.maslenkin@gmail.com>
> Fixes: 5c56dd27a2 ("qemu-nbd: fix regression with qemu-nbd --fork run over ssh")
> ---
>  qemu-nbd.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:09AM +0200, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  docs/tools/qemu-nbd.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
> index faf6349ea5..5c48ee7345 100644
> --- a/docs/tools/qemu-nbd.rst
> +++ b/docs/tools/qemu-nbd.rst
> @@ -197,7 +197,9 @@ driver options if :option:`--image-opts` is specified.
>  
>  .. option:: -v, --verbose
>  
> -  Display extra debugging information.
> +  Display extra debugging information. This option also keeps opened original
> +  *STDERR* stream if ``qemu-nbd`` process is daemonized due to other options
> +  like :option:`--fork` or :option:`-c`.

As a native speaker, I find the following a bit easier to parse:

This option also keeps the original *STDERR* stream open if ...

I can make that touchup as part of queuing the series.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 7/8] qemu-nbd: document -v behavior in respect to --fork in man

[Denis V. Lunev]

On 9/8/23 00:01, Eric Blake wrote:
> On Wed, Sep 06, 2023 at 11:32:09AM +0200, Denis V. Lunev wrote:
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   docs/tools/qemu-nbd.rst | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
>> index faf6349ea5..5c48ee7345 100644
>> --- a/docs/tools/qemu-nbd.rst
>> +++ b/docs/tools/qemu-nbd.rst
>> @@ -197,7 +197,9 @@ driver options if :option:`--image-opts` is specified.
>>   
>>   .. option:: -v, --verbose
>>   
>> -  Display extra debugging information.
>> +  Display extra debugging information. This option also keeps opened original
>> +  *STDERR* stream if ``qemu-nbd`` process is daemonized due to other options
>> +  like :option:`--fork` or :option:`-c`.
> As a native speaker, I find the following a bit easier to parse:
>
> This option also keeps the original *STDERR* stream open if ...
>
> I can make that touchup as part of queuing the series.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
That would be great, thanks!

Re: [PATCH 8/8] qemu-nbd: fix formatting in main()

[Eric Blake]

On Wed, Sep 06, 2023 at 11:32:10AM +0200, Denis V. Lunev wrote:
> Just a formatting, no functional changes.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> Do not really sure that this patch is mandatory, just stabs my eye. Feel free
> to drop if this is too useless.
> 
>  qemu-nbd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b9c74ce77c..8eb1d1f40b 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -581,7 +581,8 @@ int main(int argc, char **argv)
>      pthread_t client_thread;
>      const char *fmt = NULL;
>      Error *local_err = NULL;
> -    BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
> +    BlockdevDetectZeroesOptions detect_zeroes =
> +                                            BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;

check-patch allows code up to 90 columngs although it does advise
staying under 80.  You fixed the long line by keeping the wrapped
portion right-flushed to 80 columns; I think more typical tree-wide is
to just indent by four spaces (at least, that's what emacs suggests I
do).  But me changing what you wrote would a complete rewrite, so I'm
reluctant to include it in my upcoming pull request, although I'm not
ruling out a later cleanup (perhaps if it touches more than one
stylistic thing at once).

I'm queuing 1-7 through my NBD tree, and running another round of
iotests before sending the pull request this week.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org