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