Laszlo Ersek
2021-Sep-20 16:32 UTC
[Libguestfs] [PATCH nbdinfo v2 3/3] copy: Print debug information with human sizes
On 09/20/21 13:04, Richard W.M. Jones wrote:> --- > copy/main.c | 6 +++++- > copy/test-verbose.sh | 4 ++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/copy/main.c b/copy/main.c > index 70534b5a..15a64544 100644 > --- a/copy/main.c > +++ b/copy/main.c > @@ -39,6 +39,7 @@ > #include <libnbd.h> > > #include "ispowerof2.h" > +#include "human-size.h" > #include "version.h" > #include "nbdcopy.h" > > @@ -508,8 +509,11 @@ open_local (const char *filename, direction d) > static void > print_rw (struct rw *rw, const char *prefix, FILE *fp) > { > + char buf[HUMAN_SIZE_LONGEST]; > + > fprintf (fp, "%s: %s \"%s\"\n", prefix, rw->ops->ops_name, rw->name); > - fprintf (fp, "%s: size=%" PRIi64 "\n", prefix, rw->size); > + fprintf (fp, "%s: size=%" PRIi64 " (%s)\n", > + prefix, rw->size, human_size (buf, rw->size, NULL)); > } > > /* Default implementation of rw->ops->get_extents for backends whichHopefully rw->size is never negative here...> diff --git a/copy/test-verbose.sh b/copy/test-verbose.sh > index afd57580..4cc67d37 100755 > --- a/copy/test-verbose.sh > +++ b/copy/test-verbose.sh > @@ -28,11 +28,11 @@ requires nbdkit --version > file=test-verbose.out > cleanup_fn rm -f $file > > -$VG nbdcopy -v -- [ nbdkit null ] null: 2>$file > +$VG nbdcopy -v -- [ nbdkit memory 1M ] null: 2>$file(1) I don't understand this change. Why do we replace "null" with "memory 1M"? (Side question that I've been meaning to ask: what is this "$VG" magic?)> > cat $file > > # Check some known strings appear in the output. > grep '^nbdcopy: src: nbd_ops' $file > -grep '^nbdcopy: src: size=0' $file > +grep '^nbdcopy: src: size=1048576 (1M)' $file > grep '^nbdcopy: dst: null_ops' $file >Ah, the test case is modified at once so it generate more interesting output. Can you note that in the commit message please? Acked-by: Laszlo Ersek <lersek at redhat.com> Thanks! Laszlo
Eric Blake
2021-Sep-20 16:41 UTC
[Libguestfs] [PATCH nbdinfo v2 3/3] copy: Print debug information with human sizes
On Mon, Sep 20, 2021 at 06:32:29PM +0200, Laszlo Ersek wrote:> > +++ b/copy/test-verbose.sh > > @@ -28,11 +28,11 @@ requires nbdkit --version > > file=test-verbose.out > > cleanup_fn rm -f $file > > > > -$VG nbdcopy -v -- [ nbdkit null ] null: 2>$file > > +$VG nbdcopy -v -- [ nbdkit memory 1M ] null: 2>$file > > (1) I don't understand this change. Why do we replace "null" with > "memory 1M"? > > (Side question that I've been meaning to ask: what is this "$VG" magic?)Answering just the side question: When LIBNBD_VALGRIND is set to 1 in the environment, then $VG is set in run.in to an invocation of valgrind, optionally further wrapped by an invocation of libtool to see through any libtool wrapper script. Otherwise $VG is empty, and you run the real binary without any outer wrappers. It's actually a clever way of checking memory usage issues during 'make check-valgrind' while probing the real binary rather than accidentally running valgrind on a shell script. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2021-Sep-20 17:09 UTC
[Libguestfs] [PATCH nbdinfo v2 3/3] copy: Print debug information with human sizes
On Mon, Sep 20, 2021 at 06:32:29PM +0200, Laszlo Ersek wrote:> On 09/20/21 13:04, Richard W.M. Jones wrote: > > --- > > copy/main.c | 6 +++++- > > copy/test-verbose.sh | 4 ++-- > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/copy/main.c b/copy/main.c > > index 70534b5a..15a64544 100644 > > --- a/copy/main.c > > +++ b/copy/main.c > > @@ -39,6 +39,7 @@ > > #include <libnbd.h> > > > > #include "ispowerof2.h" > > +#include "human-size.h" > > #include "version.h" > > #include "nbdcopy.h" > > > > @@ -508,8 +509,11 @@ open_local (const char *filename, direction d) > > static void > > print_rw (struct rw *rw, const char *prefix, FILE *fp) > > { > > + char buf[HUMAN_SIZE_LONGEST]; > > + > > fprintf (fp, "%s: %s \"%s\"\n", prefix, rw->ops->ops_name, rw->name); > > - fprintf (fp, "%s: size=%" PRIi64 "\n", prefix, rw->size); > > + fprintf (fp, "%s: size=%" PRIi64 " (%s)\n", > > + prefix, rw->size, human_size (buf, rw->size, NULL)); > > } > > > > /* Default implementation of rw->ops->get_extents for backends which > > Hopefully rw->size is never negative here...About this and your similar comment on patch 2, the size is returned by this API: https://libguestfs.org/nbd_get_size.3.html This function returns int64_t because -1 indicates an error, but in non-error cases the size is always between 0 and INT64_MAX, so we use int64_t to store the size in programs just to avoid an awkward conversion. libnbd and nbdkit both assume and support only export sizes in this range, indeed we even test it: https://gitlab.com/nbdkit/nbdkit/-/blob/master/tests/test-memory-largest.sh qemu/qemu-nbd doesn't actually support the full range of sizes, and they've even {changed|broken} [delete as appropriate] what they support in recent memory, see: https://gitlab.com/nbdkit/nbdkit/-/commit/c3ec8c951e39a0f921252c162c236f23c588d2bd The NBD protocol in theory does not limit export sizes, any unsigned 64 bit size could be used, but I doubt the kernel could handle it correctly.> > diff --git a/copy/test-verbose.sh b/copy/test-verbose.sh > > index afd57580..4cc67d37 100755 > > --- a/copy/test-verbose.sh > > +++ b/copy/test-verbose.sh > > @@ -28,11 +28,11 @@ requires nbdkit --version > > file=test-verbose.out > > cleanup_fn rm -f $file > > > > -$VG nbdcopy -v -- [ nbdkit null ] null: 2>$file > > +$VG nbdcopy -v -- [ nbdkit memory 1M ] null: 2>$file > > (1) I don't understand this change. Why do we replace "null" with > "memory 1M"?I'm afraid I already pushed this change so I cannot update the commit message, but you're right below that it's to make the output more interesting. In fact I had to change it again (to size=1M) because Debian 10 is shipping some ancient nbdkit that doesn't support the abbreviated "1M" (without magic size=) form. Thanks, Rich.> (Side question that I've been meaning to ask: what is this "$VG" magic?) > > > > > cat $file > > > > # Check some known strings appear in the output. > > grep '^nbdcopy: src: nbd_ops' $file > > -grep '^nbdcopy: src: size=0' $file > > +grep '^nbdcopy: src: size=1048576 (1M)' $file > > grep '^nbdcopy: dst: null_ops' $file > > > > Ah, the test case is modified at once so it generate more interesting > output. Can you note that in the commit message please? > > Acked-by: Laszlo Ersek <lersek at redhat.com> > > Thanks! > Laszlo-- 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