[PATCH v2 0/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[PATCH v2 0/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Denis V. Lunev]

Patches 3-5 are not necessary for stable, I believe.

Changes from v1:
* added patch 1, necessary to pass fork_process into nbd_client_thread
* tweaked comment in patch 2 a bit
* added patches 3-5 with error handling improvements

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

[PATCH 1/5] qemu-nbd: pass structure into nbd_client_thread instead of plain char*

[Denis V. Lunev]

We are going to pass additional flag inside next patch.

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4276163564..77f98c736b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -272,9 +272,13 @@ static void *show_parts(void *arg)
     return NULL;
 }
 
+struct NbdClientOpts {
+    char *device;
+};
+
 static void *nbd_client_thread(void *arg)
 {
-    char *device = arg;
+    struct NbdClientOpts *opts = arg;
     NBDExportInfo info = { .request_sizes = false, .name = g_strdup("") };
     QIOChannelSocket *sioc;
     int fd = -1;
@@ -298,10 +302,10 @@ static void *nbd_client_thread(void *arg)
         goto out;
     }
 
-    fd = open(device, O_RDWR);
+    fd = open(opts->device, O_RDWR);
     if (fd < 0) {
         /* Linux-only, we can use %m in printf.  */
-        error_report("Failed to open %s: %m", device);
+        error_report("Failed to open %s: %m", opts->device);
         goto out;
     }
 
@@ -311,11 +315,11 @@ static void *nbd_client_thread(void *arg)
     }
 
     /* update partition table */
-    pthread_create(&show_parts_thread, NULL, show_parts, device);
+    pthread_create(&show_parts_thread, NULL, show_parts, opts->device);
 
     if (verbose) {
         fprintf(stderr, "NBD device %s is now connected to %s\n",
-                device, srcpath);
+                opts->device, srcpath);
     } else {
         /* Close stderr so that the qemu-nbd process exits.  */
         dup2(STDOUT_FILENO, STDERR_FILENO);
@@ -1125,8 +1129,11 @@ int main(int argc, char **argv)
     if (device) {
 #if HAVE_NBD_DEVICE
         int ret;
+        struct NbdClientOpts opts = {
+            .device = device,
+        };
 
-        ret = pthread_create(&client_thread, NULL, nbd_client_thread, device);
+        ret = pthread_create(&client_thread, NULL, nbd_client_thread, &opts);
         if (ret != 0) {
             error_report("Failed to create client thread: %s", strerror(ret));
             exit(EXIT_FAILURE);
-- 
2.34.1

[PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Denis V. Lunev]

Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
    Author: Hanna Reitz <hreitz@redhat.com>
    Date:   Wed May 8 23:18:18 2019 +0200
    qemu-nbd: Do not close stderr
has introduced an interesting regression. Original behavior of
    ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
was the following:
 * qemu-nbd was started as a daemon
 * the command execution is done and ssh exited with success

The patch has changed this behavior and 'ssh' command now hangs forever.

According to the normal specification of the daemon() call, we should
endup with STDERR pointing to /dev/null. That should be done at the
very end of the successful startup sequence when the pipe to the
bootstrap process (used for diagnostics) is no longer needed.

This could be achived in the same way as done for 'qemu-nbd -c' case.
That was commit 0eaf453e, also fixing up e6df58a5. STDOUT copying to
STDERR does the trick.

This also leads to proper 'ssh' connection closing which fixes my
original problem.

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: <qemu-stable@nongnu.org>
---
 qemu-nbd.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 77f98c736b..186ce9474c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -274,6 +274,7 @@ static void *show_parts(void *arg)
 
 struct NbdClientOpts {
     char *device;
+    bool fork_process;
 };
 
 static void *nbd_client_thread(void *arg)
@@ -317,7 +318,7 @@ static void *nbd_client_thread(void *arg)
     /* update partition table */
     pthread_create(&show_parts_thread, NULL, show_parts, opts->device);
 
-    if (verbose) {
+    if (verbose && !opts->fork_process) {
         fprintf(stderr, "NBD device %s is now connected to %s\n",
                 opts->device, srcpath);
     } else {
@@ -579,7 +580,6 @@ int main(int argc, char **argv)
     bool writethrough = false; /* Client will flush as needed. */
     bool fork_process = false;
     bool list = false;
-    int old_stderr = -1;
     unsigned socket_activation;
     const char *pid_file_name = NULL;
     const char *selinux_label = NULL;
@@ -934,11 +934,6 @@ int main(int argc, char **argv)
         } else if (pid == 0) {
             close(stderr_fd[0]);
 
-            /* Remember parent's stderr if we will be restoring it. */
-            if (fork_process) {
-                old_stderr = dup(STDERR_FILENO);
-            }
-
             ret = qemu_daemon(1, 0);
 
             /* Temporarily redirect stderr to the parent's pipe...  */
@@ -1131,6 +1126,7 @@ int main(int argc, char **argv)
         int ret;
         struct NbdClientOpts opts = {
             .device = device,
+            .fork_process = fork_process,
         };
 
         ret = pthread_create(&client_thread, NULL, nbd_client_thread, &opts);
@@ -1159,8 +1155,7 @@ int main(int argc, char **argv)
     }
 
     if (fork_process) {
-        dup2(old_stderr, STDERR_FILENO);
-        close(old_stderr);
+        dup2(STDOUT_FILENO, STDERR_FILENO);
     }
 
     state = RUNNING;
-- 
2.34.1

[PATCH 3/5] qemu-nbd: properly report error on error in dup2() after qemu_daemon()

[Denis V. Lunev]

We are trying to temporary redirect stderr of daemonized process to
a pipe to report a error and get failed. In that case we could not
use error_report() helper, but should write the message directly into
the problematic pipe.

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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 186ce9474c..4450cc826b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -937,7 +937,20 @@ int main(int argc, char **argv)
             ret = qemu_daemon(1, 0);
 
             /* Temporarily redirect stderr to the parent's pipe...  */
-            dup2(stderr_fd[1], STDERR_FILENO);
+            if (dup2(stderr_fd[1], STDERR_FILENO) < 0) {
+                char str[256];
+                snprintf(str, sizeof(str),
+                         "%s: Failed to link stderr to the pipe: %s\n",
+                         g_get_prgname(), strerror(errno));
+                /*
+                 * We are unable to use error_report() here as we need to get
+                 * stderr pointed to the parent's pipe. Write to that pipe
+                 * manually.
+                 */
+                ret = write(stderr_fd[1], str, strlen(str));
+                exit(EXIT_FAILURE);
+            }
+
             if (ret < 0) {
                 error_report("Failed to daemonize: %s", strerror(errno));
                 exit(EXIT_FAILURE);
-- 
2.34.1

[PATCH 4/5] qemu-nbd: properly report error if qemu_daemon() is failed

[Denis V. Lunev]

errno has been overwritten by dup2() just below qemu_daemon() and thus
improperly returned to the caller. Fix accordingly.

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4450cc826b..f27613cb57 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -932,9 +932,12 @@ int main(int argc, char **argv)
             error_report("Failed to fork: %s", strerror(errno));
             exit(EXIT_FAILURE);
         } else if (pid == 0) {
+            int saved_errno;
+
             close(stderr_fd[0]);
 
             ret = qemu_daemon(1, 0);
+            saved_errno = errno;    /* dup2 will overwrite error below */
 
             /* Temporarily redirect stderr to the parent's pipe...  */
             if (dup2(stderr_fd[1], STDERR_FILENO) < 0) {
@@ -952,7 +955,7 @@ int main(int argc, char **argv)
             }
 
             if (ret < 0) {
-                error_report("Failed to daemonize: %s", strerror(errno));
+                error_report("Failed to daemonize: %s", strerror(saved_errno));
                 exit(EXIT_FAILURE);
             }
 
-- 
2.34.1

[PATCH 5/5] qemu-nbd: handle dup2() error when qemu-nbd finished setup process

[Denis V. Lunev]

Fail on error, we are in trouble.

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index f27613cb57..cd0e965705 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -323,7 +323,12 @@ static void *nbd_client_thread(void *arg)
                 opts->device, srcpath);
     } else {
         /* Close stderr so that the qemu-nbd process exits.  */
-        dup2(STDOUT_FILENO, STDERR_FILENO);
+        int err = dup2(STDOUT_FILENO, STDERR_FILENO);
+        if (err < 0) {
+            error_report("Could not set stderr to /dev/null: %s",
+                         strerror(errno));
+            exit(EXIT_FAILURE);
+        }
     }
 
     if (nbd_client(fd) < 0) {
@@ -1171,7 +1176,12 @@ int main(int argc, char **argv)
     }
 
     if (fork_process) {
-        dup2(STDOUT_FILENO, STDERR_FILENO);
+        ret = dup2(STDOUT_FILENO, STDERR_FILENO);
+        if (ret < 0) {
+            error_report("Could not set stderr to /dev/null: %s",
+                         strerror(errno));
+            exit(EXIT_FAILURE);
+        }
     }
 
     state = RUNNING;
-- 
2.34.1

Re: [PATCH 1/5] qemu-nbd: pass structure into nbd_client_thread instead of plain char*

[Eric Blake]

On Mon, Jul 17, 2023 at 04:55:40PM +0200, Denis V. Lunev wrote:
> We are going to pass additional flag inside next patch.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> CC: <qemu-stable@nongnu.org>
> ---
>  qemu-nbd.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)

We could also have just promoted any additional variables to be shared
between the parent and child from stack-local in main() to global; but
this refactoring seems cleaner.

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 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Eric Blake]

On Mon, Jul 17, 2023 at 04:55:41PM +0200, Denis V. Lunev wrote:
> Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
>     Author: Hanna Reitz <hreitz@redhat.com>
>     Date:   Wed May 8 23:18:18 2019 +0200
>     qemu-nbd: Do not close stderr
> has introduced an interesting regression. Original behavior of
>     ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
> was the following:
>  * qemu-nbd was started as a daemon
>  * the command execution is done and ssh exited with success
> 
> The patch has changed this behavior and 'ssh' command now hangs forever.
> 
> According to the normal specification of the daemon() call, we should
> endup with STDERR pointing to /dev/null. That should be done at the
> very end of the successful startup sequence when the pipe to the
> bootstrap process (used for diagnostics) is no longer needed.
> 
> This could be achived in the same way as done for 'qemu-nbd -c' case.
> That was commit 0eaf453e, also fixing up e6df58a5. STDOUT copying to
> STDERR does the trick.
> 
> This also leads to proper 'ssh' connection closing which fixes my
> original problem.
> 
> 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: <qemu-stable@nongnu.org>
> ---
>  qemu-nbd.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 77f98c736b..186ce9474c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -274,6 +274,7 @@ static void *show_parts(void *arg)
>  
>  struct NbdClientOpts {
>      char *device;
> +    bool fork_process;
>  };
>  
>  static void *nbd_client_thread(void *arg)
> @@ -317,7 +318,7 @@ static void *nbd_client_thread(void *arg)
>      /* update partition table */
>      pthread_create(&show_parts_thread, NULL, show_parts, opts->device);
>  
> -    if (verbose) {
> +    if (verbose && !opts->fork_process) {

It seems a bit odd to use the global 'fork' but the local
'opts->fork_process' in the same conditional.  Perhaps patch 1/5
should be modified to also pass verbose through opts?

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

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

[PATCH 6/5] qemu-nbd: make verbose bool and local variable in main()

[Denis V. Lunev]

Pass 'verbose' to nbd_client_thread() inside NbdClientOpts which looks
a little bit cleaner and make it bool as it is used as bool actually.

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index cd0e965705..958e5688c0 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -73,7 +73,6 @@
 
 #define MBR_SIZE 512
 
-static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
 static int persistent = 0;
@@ -275,6 +274,7 @@ static void *show_parts(void *arg)
 struct NbdClientOpts {
     char *device;
     bool fork_process;
+    bool verbose;
 };
 
 static void *nbd_client_thread(void *arg)
@@ -318,7 +318,7 @@ static void *nbd_client_thread(void *arg)
     /* update partition table */
     pthread_create(&show_parts_thread, NULL, show_parts, opts->device);
 
-    if (verbose && !opts->fork_process) {
+    if (opts->verbose && !opts->fork_process) {
         fprintf(stderr, "NBD device %s is now connected to %s\n",
                 opts->device, srcpath);
     } else {
@@ -583,6 +583,7 @@ 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;
@@ -747,7 +748,7 @@ int main(int argc, char **argv)
             }
             break;
         case 'v':
-            verbose = 1;
+            verbose = true;
             break;
         case 'V':
             version(argv[0]);
@@ -1148,6 +1149,7 @@ int main(int argc, char **argv)
         struct NbdClientOpts opts = {
             .device = device,
             .fork_process = fork_process,
+            .verbose = verbose,
         };
 
         ret = pthread_create(&client_thread, NULL, nbd_client_thread, &opts);
-- 
2.34.1

Re: [PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Denis V. Lunev]

On 7/17/23 21:04, Eric Blake wrote:
> On Mon, Jul 17, 2023 at 04:55:41PM +0200, Denis V. Lunev wrote:
>> Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
>>      Author: Hanna Reitz <hreitz@redhat.com>
>>      Date:   Wed May 8 23:18:18 2019 +0200
>>      qemu-nbd: Do not close stderr
>> has introduced an interesting regression. Original behavior of
>>      ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
>> was the following:
>>   * qemu-nbd was started as a daemon
>>   * the command execution is done and ssh exited with success
>>
>> The patch has changed this behavior and 'ssh' command now hangs forever.
>>
>> According to the normal specification of the daemon() call, we should
>> endup with STDERR pointing to /dev/null. That should be done at the
>> very end of the successful startup sequence when the pipe to the
>> bootstrap process (used for diagnostics) is no longer needed.
>>
>> This could be achived in the same way as done for 'qemu-nbd -c' case.
>> That was commit 0eaf453e, also fixing up e6df58a5. STDOUT copying to
>> STDERR does the trick.
>>
>> This also leads to proper 'ssh' connection closing which fixes my
>> original problem.
>>
>> 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: <qemu-stable@nongnu.org>
>> ---
>>   qemu-nbd.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 77f98c736b..186ce9474c 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -274,6 +274,7 @@ static void *show_parts(void *arg)
>>   
>>   struct NbdClientOpts {
>>       char *device;
>> +    bool fork_process;
>>   };
>>   
>>   static void *nbd_client_thread(void *arg)
>> @@ -317,7 +318,7 @@ static void *nbd_client_thread(void *arg)
>>       /* update partition table */
>>       pthread_create(&show_parts_thread, NULL, show_parts, opts->device);
>>   
>> -    if (verbose) {
>> +    if (verbose && !opts->fork_process) {
> It seems a bit odd to use the global 'fork' but the local
> 'opts->fork_process' in the same conditional.  Perhaps patch 1/5
> should be modified to also pass verbose through opts?
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
sent to the thread as [PATCH 6/5] for convenience

Den

Re: [PATCH 3/5] qemu-nbd: properly report error on error in dup2() after qemu_daemon()

[Eric Blake]

On Mon, Jul 17, 2023 at 04:55:42PM +0200, Denis V. Lunev wrote:
> We are trying to temporary redirect stderr of daemonized process to

temporarily

> a pipe to report a error and get failed. In that case we could not
> use error_report() helper, but should write the message directly into
> the problematic pipe.
> 
> 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 | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 186ce9474c..4450cc826b 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -937,7 +937,20 @@ int main(int argc, char **argv)
>              ret = qemu_daemon(1, 0);
>

errno from qemu_daemon...

>              /* Temporarily redirect stderr to the parent's pipe...  */
> -            dup2(stderr_fd[1], STDERR_FILENO);
> +            if (dup2(stderr_fd[1], STDERR_FILENO) < 0) {

...is still potentially lost here...

> +                char str[256];
> +                snprintf(str, sizeof(str),
> +                         "%s: Failed to link stderr to the pipe: %s\n",
> +                         g_get_prgname(), strerror(errno));
> +                /*
> +                 * We are unable to use error_report() here as we need to get
> +                 * stderr pointed to the parent's pipe. Write to that pipe
> +                 * manually.
> +                 */
> +                ret = write(stderr_fd[1], str, strlen(str));
> +                exit(EXIT_FAILURE);
> +            }
> +
>              if (ret < 0) {
>                  error_report("Failed to daemonize: %s", strerror(errno));

...before use here.

Patch 4/5 addresses that, but I'm inclined to just swap the order of
the two patches when applying the series in my NBD tree.

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 4/5] qemu-nbd: properly report error if qemu_daemon() is failed

[Eric Blake]

On Mon, Jul 17, 2023 at 04:55:43PM +0200, Denis V. Lunev wrote:
> errno has been overwritten by dup2() just below qemu_daemon() and thus
> improperly returned to the caller. Fix accordingly.
> 
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

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

> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 4450cc826b..f27613cb57 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -932,9 +932,12 @@ int main(int argc, char **argv)
>              error_report("Failed to fork: %s", strerror(errno));
>              exit(EXIT_FAILURE);
>          } else if (pid == 0) {
> +            int saved_errno;
> +
>              close(stderr_fd[0]);
>  
>              ret = qemu_daemon(1, 0);
> +            saved_errno = errno;    /* dup2 will overwrite error below */

I would have written 'may', not 'will'; but that's a triviality not
worth changing.


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

Re: [PATCH 5/5] qemu-nbd: handle dup2() error when qemu-nbd finished setup process

[Eric Blake]

On Mon, Jul 17, 2023 at 04:55:44PM +0200, Denis V. Lunev wrote:
> Fail on error, we are in trouble.
> 
> 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 | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index f27613cb57..cd0e965705 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -323,7 +323,12 @@ static void *nbd_client_thread(void *arg)
>                  opts->device, srcpath);
>      } else {
>          /* Close stderr so that the qemu-nbd process exits.  */
> -        dup2(STDOUT_FILENO, STDERR_FILENO);
> +        int err = dup2(STDOUT_FILENO, STDERR_FILENO);
> +        if (err < 0) {

Shorter to drop the temporary variable, and just do:

   if (dup2(...) < 0) {

> +            error_report("Could not set stderr to /dev/null: %s",
> +                         strerror(errno));
> +            exit(EXIT_FAILURE);
> +        }
>      }

Either way,
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 6/5] qemu-nbd: make verbose bool and local variable in main()

[Eric Blake]

On Mon, Jul 17, 2023 at 10:25:20PM +0200, Denis V. Lunev wrote:
> Pass 'verbose' to nbd_client_thread() inside NbdClientOpts which looks
> a little bit cleaner and make it bool as it is used as bool actually.
> 
> 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

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

I'll queue the series through my NBD tree.

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

Re: [PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Kevin Wolf]

Am 17.07.2023 um 16:55 hat Denis V. Lunev geschrieben:
> Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
>     Author: Hanna Reitz <hreitz@redhat.com>
>     Date:   Wed May 8 23:18:18 2019 +0200
>     qemu-nbd: Do not close stderr
> has introduced an interesting regression. Original behavior of
>     ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
> was the following:
>  * qemu-nbd was started as a daemon
>  * the command execution is done and ssh exited with success
> 
> The patch has changed this behavior and 'ssh' command now hangs forever.
> 
> According to the normal specification of the daemon() call, we should
> endup with STDERR pointing to /dev/null. That should be done at the
> very end of the successful startup sequence when the pipe to the
> bootstrap process (used for diagnostics) is no longer needed.
> 
> This could be achived in the same way as done for 'qemu-nbd -c' case.
> That was commit 0eaf453e, also fixing up e6df58a5. STDOUT copying to
> STDERR does the trick.
> 
> This also leads to proper 'ssh' connection closing which fixes my
> original problem.
> 
> 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: <qemu-stable@nongnu.org>

This broke qemu-iotests 233 (Eric, please make sure to run the full
qemu-iotests suite before sending block related pull requests):

--- /home/kwolf/source/qemu/tests/qemu-iotests/233.out
+++ /home/kwolf/source/qemu/build-clang/scratch/raw-file-233/233.out.bad
@@ -99,14 +99,4 @@
 qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated.

 == final server log ==
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
-qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
-qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
-qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
-qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
 *** done

Do we really want to lose these error messages? This looks wrong to me.

Kevin

Re: [PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Denis V. Lunev]

On 8/14/23 16:14, Kevin Wolf wrote:
> Am 17.07.2023 um 16:55 hat Denis V. Lunev geschrieben:
>> Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
>>      Author: Hanna Reitz <hreitz@redhat.com>
>>      Date:   Wed May 8 23:18:18 2019 +0200
>>      qemu-nbd: Do not close stderr
>> has introduced an interesting regression. Original behavior of
>>      ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
>> was the following:
>>   * qemu-nbd was started as a daemon
>>   * the command execution is done and ssh exited with success
>>
>> The patch has changed this behavior and 'ssh' command now hangs forever.
>>
>> According to the normal specification of the daemon() call, we should
>> endup with STDERR pointing to /dev/null. That should be done at the
>> very end of the successful startup sequence when the pipe to the
>> bootstrap process (used for diagnostics) is no longer needed.
>>
>> This could be achived in the same way as done for 'qemu-nbd -c' case.
>> That was commit 0eaf453e, also fixing up e6df58a5. STDOUT copying to
>> STDERR does the trick.
>>
>> This also leads to proper 'ssh' connection closing which fixes my
>> original problem.
>>
>> 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: <qemu-stable@nongnu.org>
> This broke qemu-iotests 233 (Eric, please make sure to run the full
> qemu-iotests suite before sending block related pull requests):
>
> --- /home/kwolf/source/qemu/tests/qemu-iotests/233.out
> +++ /home/kwolf/source/qemu/build-clang/scratch/raw-file-233/233.out.bad
> @@ -99,14 +99,4 @@
>   qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated.
>
>   == final server log ==
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
> -qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
> -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
> -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
> -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
>   *** done
>
> Do we really want to lose these error messages? This looks wrong to me.
>
> Kevin
>
In that case likely we need to extend -v option behavior
and translate these messages in that case.

I'll take a look.

Thank you for the patience,
     Den

Re: [PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Denis V. Lunev]

On 8/15/23 12:40, Denis V. Lunev wrote:
> On 8/14/23 16:14, Kevin Wolf wrote:
>> Am 17.07.2023 um 16:55 hat Denis V. Lunev geschrieben:
>>> Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
>>>      Author: Hanna Reitz <hreitz@redhat.com>
>>>      Date:   Wed May 8 23:18:18 2019 +0200
>>>      qemu-nbd: Do not close stderr
>>> has introduced an interesting regression. Original behavior of
>>>      ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
>>> was the following:
>>>   * qemu-nbd was started as a daemon
>>>   * the command execution is done and ssh exited with success
>>>
>>> The patch has changed this behavior and 'ssh' command now hangs 
>>> forever.
>>>
>>> According to the normal specification of the daemon() call, we should
>>> endup with STDERR pointing to /dev/null. That should be done at the
>>> very end of the successful startup sequence when the pipe to the
>>> bootstrap process (used for diagnostics) is no longer needed.
>>>
>>> This could be achived in the same way as done for 'qemu-nbd -c' case.
>>> That was commit 0eaf453e, also fixing up e6df58a5. STDOUT copying to
>>> STDERR does the trick.
>>>
>>> This also leads to proper 'ssh' connection closing which fixes my
>>> original problem.
>>>
>>> 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: <qemu-stable@nongnu.org>
>> This broke qemu-iotests 233 (Eric, please make sure to run the full
>> qemu-iotests suite before sending block related pull requests):
>>
>> --- /home/kwolf/source/qemu/tests/qemu-iotests/233.out
>> +++ /home/kwolf/source/qemu/build-clang/scratch/raw-file-233/233.out.bad
>> @@ -99,14 +99,4 @@
>>   qemu-nbd: TLS handshake failed: The TLS connection was non-properly 
>> terminated.
>>
>>   == final server log ==
>> -qemu-nbd: option negotiation failed: Failed to read opts magic: 
>> Cannot read from TLS channel: Software caused connection abort
>> -qemu-nbd: option negotiation failed: Failed to read opts magic: 
>> Cannot read from TLS channel: Software caused connection abort
>> -qemu-nbd: option negotiation failed: Verify failed: No certificate 
>> was found.
>> -qemu-nbd: option negotiation failed: Verify failed: No certificate 
>> was found.
>> -qemu-nbd: option negotiation failed: TLS x509 authz check for 
>> DISTINGUISHED-NAME is denied
>> -qemu-nbd: option negotiation failed: TLS x509 authz check for 
>> DISTINGUISHED-NAME is denied
>> -qemu-nbd: option negotiation failed: Failed to read opts magic: 
>> Cannot read from TLS channel: Software caused connection abort
>> -qemu-nbd: option negotiation failed: Failed to read opts magic: 
>> Cannot read from TLS channel: Software caused connection abort
>> -qemu-nbd: option negotiation failed: TLS handshake failed: An 
>> illegal parameter has been received.
>> -qemu-nbd: option negotiation failed: TLS handshake failed: An 
>> illegal parameter has been received.
>>   *** done
>>
>> Do we really want to lose these error messages? This looks wrong to me.
>>
>> Kevin
>>
> In that case likely we need to extend -v option behavior
> and translate these messages in that case.
>
> I'll take a look.
>
> Thank you for the patience,
>     Den

Hi!

Small side note.

I am 100% sure that I have run this set of tests and
there was no fault. I have re-run them and once
again has not get the fault :-)

The reason for that is quite interesting:
* the test does not start due to the absence of the
   'certool' utility from gnutls

This brings the very important question.

Should we *FAIL* when important utility is missed
or skip? I believe that our goal is to fail to avoid such
cases.

What do you think?

Den

Re: [PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Kevin Wolf]

Am 15.08.2023 um 14:17 hat Denis V. Lunev geschrieben:
> Hi!
> 
> Small side note.
> 
> I am 100% sure that I have run this set of tests and
> there was no fault. I have re-run them and once
> again has not get the fault :-)
> 
> The reason for that is quite interesting:
> * the test does not start due to the absence of the
>   'certool' utility from gnutls
> 
> This brings the very important question.
> 
> Should we *FAIL* when important utility is missed
> or skip? I believe that our goal is to fail to avoid such
> cases.
> 
> What do you think?

In general I think it makes sense that FAIL means that the test could
run as expected, but we got an unexpected result (i.e. this is likely a
QEMU bug), and SKIP means that the test couldn't meaningfully be
performed on the host system.

Making more things hard dependencies for the test would mean that it's
harder to miss an instance like this, but it would also make it harder
to run the test suite on a system that doesn't have the dependencies
available (and you might not even have root privileges to install them).

I think I'd leave things as they are now, but recommend that you
occasionally check the tests reported as "not run" to see if you could
easily provide the thing they would need.

Kevin

Re: [PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Peter Maydell]

On Tue, 15 Aug 2023 at 15:59, Kevin Wolf <kwolf@redhat.com> wrote:
> In general I think it makes sense that FAIL means that the test could
> run as expected, but we got an unexpected result (i.e. this is likely a
> QEMU bug), and SKIP means that the test couldn't meaningfully be
> performed on the host system.
>
> Making more things hard dependencies for the test would mean that it's
> harder to miss an instance like this, but it would also make it harder
> to run the test suite on a system that doesn't have the dependencies
> available (and you might not even have root privileges to install them).
>
> I think I'd leave things as they are now, but recommend that you
> occasionally check the tests reported as "not run" to see if you could
> easily provide the thing they would need.

In a utopian world we might have a "make query-test-dependencies"
or something that would list all the tools/dependencies/etc that
are missing and which tests this will cause to be skipped...

-- PMM

Re: [PATCH 2/5] qemu-nbd: fix regression with qemu-nbd --fork run over ssh

[Eric Blake]

On Mon, Aug 14, 2023 at 04:14:41PM +0200, Kevin Wolf wrote:
> Am 17.07.2023 um 16:55 hat Denis V. Lunev geschrieben:
> > Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
> >     Author: Hanna Reitz <hreitz@redhat.com>
> >     Date:   Wed May 8 23:18:18 2019 +0200
> >     qemu-nbd: Do not close stderr
> > has introduced an interesting regression. Original behavior of
> >     ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
> > was the following:
> >  * qemu-nbd was started as a daemon
> >  * the command execution is done and ssh exited with success

Thinking about this more...

The original problem is that we broke 'ssh -c "qemu-nbd --fork ..."',
because the daemonized process hung on to the parent's stderr
indefinitely.

But when we pass -v, we WANT the parent's stderr to hang around, even
while we still want the parent process to see EOF on the handshake
socket used to let it know the child process got far enough along in
its initialization.

Should we be passing 'opt->verbose' instead of '0' to the second
parameter of qemu_daemon, to tell the child process the scenarios
where we want output to still be present?  If so, how does the
following patch look?

diff --git i/qemu-nbd.c w/qemu-nbd.c
index aaccaa33184..c316a91831d 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -944,9 +944,24 @@ int main(int argc, char **argv)

             close(stderr_fd[0]);

-            ret = qemu_daemon(1, 0);
+            ret = qemu_daemon(1, verbose);
             saved_errno = errno;    /* dup2 will overwrite error below */

+            if (verbose) {
+                /* We want stdin at /dev/null when qemu_daemon didn't do it */
+                stdin = freopen ("/dev/null", "r", stdin);
+                if (stdin == NULL) {
+                    error_report("Failed to redirect stdin: %s",
+                                 strerror(errno));
+                    exit(EXIT_FAILURE);
+                }
+                /* To keep the parent's stderr alive, copy it to stdout */
+                if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
+                    error_report("Failed to redirect stdout: %s",
+                                 strerror(errno));
+                    exit(EXIT_FAILURE);
+                }
+            }
             /* Temporarily redirect stderr to the parent's pipe...  */
             if (dup2(stderr_fd[1], STDERR_FILENO) < 0) {
                 char str[256];
@@ -1180,6 +1195,10 @@ int main(int argc, char **argv)
     }

     if (fork_process) {
+        /*
+         * See above. If verbose is false, stdout is /dev/null (thanks
+         * to qemu_daemon); otherwise, stdout is the parent's stderr.
+         */
         if (dup2(STDOUT_FILENO, STDERR_FILENO) < 0) {
             error_report("Could not set stderr to /dev/null: %s",
                          strerror(errno));


Note, however, that this still does not pass test 233 as written - the
error messages show up earlier in the run, rather than disappearing
altogether.

> > 
> > The patch has changed this behavior and 'ssh' command now hangs forever.
> > 
> > According to the normal specification of the daemon() call, we should
> > endup with STDERR pointing to /dev/null. That should be done at the
> > very end of the successful startup sequence when the pipe to the
> > bootstrap process (used for diagnostics) is no longer needed.
> > 
> > This could be achived in the same way as done for 'qemu-nbd -c' case.
> > That was commit 0eaf453e, also fixing up e6df58a5. STDOUT copying to
> > STDERR does the trick.
> > 
> > This also leads to proper 'ssh' connection closing which fixes my
> > original problem.
> > 
> > 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: <qemu-stable@nongnu.org>
> 
> This broke qemu-iotests 233 (Eric, please make sure to run the full
> qemu-iotests suite before sending block related pull requests):

My apologies; I keep forgetting that './check -nbd' does not catch all
the possible tests using NBD.  I've updated my checklists to make sure
I'm running a more thorough set of iotests before preparing a pull
request.

> 
> --- /home/kwolf/source/qemu/tests/qemu-iotests/233.out
> +++ /home/kwolf/source/qemu/build-clang/scratch/raw-file-233/233.out.bad
> @@ -99,14 +99,4 @@
>  qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated.
> 
>  == final server log ==
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
> -qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
> -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
> -qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
> -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
> -qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
>  *** done
> 
> Do we really want to lose these error messages? This looks wrong to me.
> 
> Kevin
> 

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