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