[libnbd PATCH] copy: Write progress to stderr, not /dev/tty
dd writes progress messages to stderr, with no complaints. Having nbdcopy open /dev/tty instead of reusing stderr to write messages litters the output of a terminal during an interactive 'make check', which is rather distracting, even though that test also ran a second copy using a different fd for inspecting the output. Fixes: 1df1b05a ("New nbdcopy tool", v1.3.8) --- copy/copy-progress-bar.sh | 4 ++-- copy/progress.c | 12 ++---------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/copy/copy-progress-bar.sh b/copy/copy-progress-bar.sh index 00380d17..4a0b9a3b 100755 --- a/copy/copy-progress-bar.sh +++ b/copy/copy-progress-bar.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # nbd client library in userspace -# Copyright (C) 2020 Red Hat Inc. +# Copyright (C) 2020-2022 Red Hat Inc. # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -32,7 +32,7 @@ cleanup_fn rm -f $file $file2 $file3 dd if=/dev/urandom of=$file bs=512 count=1 # Check that a regular progress bar works. -# This writes to /dev/tty :-) +# This writes to stderr $VG nbdcopy --progress $file $file2 # Check that a machine-readable progress bar works. diff --git a/copy/progress.c b/copy/progress.c index a8b1a470..c1a873c9 100644 --- a/copy/progress.c +++ b/copy/progress.c @@ -1,5 +1,5 @@ /* NBD client library in userspace. - * Copyright (C) 2020 Red Hat Inc. + * Copyright (C) 2020-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -37,8 +37,6 @@ static void do_progress_bar (off_t pos, int64_t size) { - static int tty = -1; - /* Note the spinner is covered with the cursor which usually makes * it appear inverse video. */ @@ -50,12 +48,6 @@ do_progress_bar (off_t pos, int64_t size) char msg[80]; size_t n, i; - if (tty == -1) { - tty = open ("/dev/tty", O_WRONLY); - if (tty == -1) - return; - } - if (frac < 0) frac = 0; else if (frac > 1) frac = 1; if (frac == 1) { @@ -75,7 +67,7 @@ do_progress_bar (off_t pos, int64_t size) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-result" - write (tty, msg, strlen (msg)); + write (fileno (stderr), msg, strlen (msg)); #pragma GCC diagnostic pop } -- 2.37.2
On Fri, Sep 02, 2022 at 10:25:38AM -0500, Eric Blake wrote:
dd writes progress messages to stderr, with no complaints. Having nbdcopy open /dev/tty instead of reusing stderr to write messages litters the output of a terminal during an interactive 'make check', which is rather distracting, even though that test also ran a second copy using a different fd for inspecting the output.
Fixes: 1df1b05a ("New nbdcopy tool", v1.3.8)
copy/copy-progress-bar.sh | 4 ++-- copy/progress.c | 12 ++---------- 2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/copy/copy-progress-bar.sh b/copy/copy-progress-bar.sh index 00380d17..4a0b9a3b 100755 --- a/copy/copy-progress-bar.sh +++ b/copy/copy-progress-bar.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # nbd client library in userspace -# Copyright (C) 2020 Red Hat Inc. +# Copyright (C) 2020-2022 Red Hat Inc. # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -32,7 +32,7 @@ cleanup_fn rm -f $file $file2 $file3 dd if=/dev/urandom of=$file bs=512 count=1
# Check that a regular progress bar works. -# This writes to /dev/tty :-) +# This writes to stderr $VG nbdcopy --progress $file $file2
# Check that a machine-readable progress bar works. diff --git a/copy/progress.c b/copy/progress.c index a8b1a470..c1a873c9 100644 --- a/copy/progress.c +++ b/copy/progress.c @@ -1,5 +1,5 @@ /* NBD client library in userspace. - * Copyright (C) 2020 Red Hat Inc. + * Copyright (C) 2020-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -37,8 +37,6 @@ static void do_progress_bar (off_t pos, int64_t size) { - static int tty = -1; - /* Note the spinner is covered with the cursor which usually makes * it appear inverse video. */ @@ -50,12 +48,6 @@ do_progress_bar (off_t pos, int64_t size) char msg[80]; size_t n, i;
- if (tty == -1) { - tty = open ("/dev/tty", O_WRONLY); - if (tty == -1) - return; - } - if (frac < 0) frac = 0; else if (frac > 1) frac = 1;
if (frac == 1) { @@ -75,7 +67,7 @@ do_progress_bar (off_t pos, int64_t size)
#pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-result" - write (tty, msg, strlen (msg)); + write (fileno (stderr), msg, strlen (msg)); #pragma GCC diagnostic pop }
ACK Thanks for looking at this, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Fri, Sep 02, 2022 at 06:58:53PM +0100, Richard W.M. Jones wrote:
On Fri, Sep 02, 2022 at 10:25:38AM -0500, Eric Blake wrote:
dd writes progress messages to stderr, with no complaints. Having nbdcopy open /dev/tty instead of reusing stderr to write messages litters the output of a terminal during an interactive 'make check', which is rather distracting, even though that test also ran a second copy using a different fd for inspecting the output.
Fixes: 1df1b05a ("New nbdcopy tool", v1.3.8)
ACK
Thanks for looking at this,
Now in as d4d4e70d -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 09/02/22 21:53, Eric Blake wrote:
On Fri, Sep 02, 2022 at 06:58:53PM +0100, Richard W.M. Jones wrote:
On Fri, Sep 02, 2022 at 10:25:38AM -0500, Eric Blake wrote:
dd writes progress messages to stderr, with no complaints. Having nbdcopy open /dev/tty instead of reusing stderr to write messages litters the output of a terminal during an interactive 'make check', which is rather distracting, even though that test also ran a second copy using a different fd for inspecting the output.
Fixes: 1df1b05a ("New nbdcopy tool", v1.3.8)
ACK
Thanks for looking at this,
Now in as d4d4e70d
How does this affect the situation described in <https://bugzilla.redhat.com/show_bug.cgi?id=2115621>? I feel like we should add a comment there, noting that commit d4d4e70d changes the behavior described previously. I'm unsure about the specifics though. Laszlo
On Mon, Sep 05, 2022 at 11:01:52AM +0200, Laszlo Ersek wrote:
On 09/02/22 21:53, Eric Blake wrote:
On Fri, Sep 02, 2022 at 06:58:53PM +0100, Richard W.M. Jones wrote:
On Fri, Sep 02, 2022 at 10:25:38AM -0500, Eric Blake wrote:
dd writes progress messages to stderr, with no complaints. Having nbdcopy open /dev/tty instead of reusing stderr to write messages litters the output of a terminal during an interactive 'make check', which is rather distracting, even though that test also ran a second copy using a different fd for inspecting the output.
Fixes: 1df1b05a ("New nbdcopy tool", v1.3.8)
ACK
Thanks for looking at this,
Now in as d4d4e70d
How does this affect the situation described in <https://bugzilla.redhat.com/show_bug.cgi?id=2115621>?
I feel like we should add a comment there, noting that commit d4d4e70d changes the behavior described previously. I'm unsure about the specifics though.
I didn't mention the progress bar in my brain dumps in that bug, but that was indeed yet another quirk of virt-v2v output. The progress bar (only) went to /dev/tty, for no particular reason. This leads to odd effects: $ virt-v2v -i disk /var/tmp/fedora-35.img -o null >& /dev/null █ 100% [****************************************] (See also the output when you run make check in virt-v2v) After this change to nbdcopy it goes to stderr so will be mixed in with the other debug messages. We probably want the progress bar to be either suppressed or changed _if_ debugging is enabled. eg. We could change it to be XX/100 instead of a "graphical" bar. Or just get rid of the -p flag. If debugging is disabled it's unclear if the progress bar really should be going to stderr, or to stdout with the other normal status messages, but this is still an improvement vs /dev/tty. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones