Re: [PULL 12/13] qemu-nbd: Restore "qemu-nbd -v --fork" output

[Stefan Hajnoczi]

Please resolve the following CI failure:

https://gitlab.com/qemu-project/qemu/-/jobs/5045998355

ninja: job failed: cc -m64 -mcx16 -Iqemu-nbd.p -I. -I.. -Iqapi -Itrace
-Iui -Iui/shader -I/usr/include/p11-kit-1 -I/usr/include/glib-2.0
-I/usr/lib/glib-2.0/include -fdiagnostics-color=auto -Wall
-Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong
-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wundef -Wwrite-strings
-Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls
-Wold-style-declaration -Wold-style-definition -Wtype-limits
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined
-Wimplicit-fallthrough=2 -Wmissing-format-attribute
-Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi
-isystem /builds/qemu-project/qemu/linux-headers -isystem
linux-headers -iquote . -iquote /builds/qemu-project/qemu -iquote
/builds/qemu-project/qemu/include -iquote
/builds/qemu-project/qemu/host/include/x86_64 -iquote
/builds/qemu-project/qemu/host/include/generic -iquote
/builds/qemu-project/qemu/tcg/i386 -pthread -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
-fno-common -fwrapv -fPIE -MD -MQ qemu-nbd.p/qemu-nbd.c.o -MF
qemu-nbd.p/qemu-nbd.c.o.d -o qemu-nbd.p/qemu-nbd.c.o -c ../qemu-nbd.c
In file included from /usr/include/fortify/stdio.h:23,
from ../include/qemu/osdep.h:110,
from ../qemu-nbd.c:19:
../qemu-nbd.c: In function 'nbd_client_thread':
../qemu-nbd.c:340:39: error: expected identifier before '(' token
340 | nbd_client_release_pipe(opts->stderr);
| ^~~~~~
../qemu-nbd.c: In function 'main':
../qemu-nbd.c:605:10: error: expected identifier before '(' token
605 | .stderr = STDOUT_FILENO,
| ^~~~~~
../qemu-nbd.c:962:22: error: expected identifier before '(' token
962 | opts.stderr = dup(STDERR_FILENO);
| ^~~~~~
../qemu-nbd.c:963:26: error: expected identifier before '(' token
963 | if (opts.stderr < 0) {
| ^~~~~~
../qemu-nbd.c:1200:38: error: expected identifier before '(' token
1200 | nbd_client_release_pipe(opts.stderr);
| ^~~~~~

On Thu, 7 Sept 2023 at 21:37, Eric Blake <eblake@redhat.com> wrote:
>
> From: "Denis V. Lunev" <den@openvz.org>
>
> 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")
> Message-ID: <20230906093210.339585-7-den@openvz.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Tested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-nbd.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 7c4e22def17..1cdc41ed292 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.41.0
>
>