Libguestfs - Sep 2019 - [libnbd PATCH] nbdsh: Add -b option to simplify h.block_status

We decided to not request the "base:allocation" context by default (if
a client wants to use block_status on a different context, then they'd
have to get any default request out of the way); however, block status
is useless without at least one meta context.  This adds a convenience
knob for requesting that, and has the nice benefit of working with the
--connect command line option (previously, if you wanted to use
block_status, you could not use --connect, because requesting the meta
context must happen before the connection).
---
 sh/nbdsh.pod       |  9 +++++++
 python/nbdsh.py    |  4 +++
 sh/Makefile.am     |  4 ++-
 sh/test-context.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100755 sh/test-context.sh

diff --git a/sh/nbdsh.pod b/sh/nbdsh.pod
index d7fc315..6c540c7 100644
--- a/sh/nbdsh.pod
+++ b/sh/nbdsh.pod
@@ -56,6 +56,15 @@ __EXAMPLES_HEXDUMP__

 Display brief command line help and exit.

+=item B<-b>
+
+=item B<--base-allocation>
+
+Request the use of the "base:allocation" meta context, which is the
+most common context used with L<nbd_block_status(3)>.  This is
+equivalent to calling S<C<h.set_meta_context
+(nbd.CONTEXT_BASE_ALLOCATION)>> in the shell prior to connecting.
+
 =item B<-c> 'COMMAND ...'

 =item B<--command> 'COMMAND ...'
diff --git a/python/nbdsh.py b/python/nbdsh.py
index 319b0f0..00bc6bc 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -27,6 +27,8 @@ def shell():
     epilog = '''Please read the nbdsh(1) manual page for full
usage.'''
     parser = argparse.ArgumentParser (prog='nbdsh',
description=description,
                                       epilog=epilog)
+    parser.add_argument ('-b', '--base-allocation',
action='store_true',
+                         help='request the "base:allocation" meta
context')
     parser.add_argument ('--connect', metavar='URI',
                          help="connect to NBD URI")
     parser.add_argument ('-c', '--command',
action='append',
@@ -52,6 +54,8 @@ exit() or Ctrl-D                    # Quit the shell
 help (nbd)                          # Display documentation
 '''

+    if args.base_allocation:
+        h.add_meta_context (nbd.CONTEXT_BASE_ALLOCATION)
     if args.connect is not None:
         h.connect_uri (args.connect)
     # If there are no -c or --command parameters, go interactive,
diff --git a/sh/Makefile.am b/sh/Makefile.am
index 8f70e69..415e241 100644
--- a/sh/Makefile.am
+++ b/sh/Makefile.am
@@ -21,6 +21,7 @@ EXTRA_DIST = \
 	nbdsh.pod \
 	examples/LICENSE-FOR-EXAMPLES \
 	examples/hexdump.sh \
+	test-context.sh \
 	test-help.sh \
 	test-pattern.sh \
 	test-version.sh \
@@ -43,7 +44,7 @@ nbdsh.1: nbdsh.pod $(top_builddir)/podwrapper.pl

 endif HAVE_POD

-TESTS_ENVIRONMENT = LIBNBD_DEBUG=1 EXP_VERSION=$(VERSION)
+TESTS_ENVIRONMENT = LIBNBD_DEBUG=1 NBDKIT_DEBUG=1 EXP_VERSION=$(VERSION)
 LOG_COMPILER = $(top_builddir)/run
 TESTS = \
 	test-help.sh \
@@ -53,6 +54,7 @@ TESTS = \
 if HAVE_NBDKIT

 TESTS += \
+	test-context.sh \
 	test-pattern.sh \
 	$(NULL)

diff --git a/sh/test-context.sh b/sh/test-context.sh
new file mode 100755
index 0000000..55f9b53
--- /dev/null
+++ b/sh/test-context.sh
@@ -0,0 +1,62 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2019 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
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+# Test effects of nbdsh -b
+fail=0
+
+# Without -b, no meta context is requested
+output=$(nbdkit -U - null --run 'nbdsh \
+    --connect "nbd+unix://?socket=$unixsocket" \
+    -c "print (h.can_meta_context (nbd.CONTEXT_BASE_ALLOCATION))"
</dev/null')
+if test "x$output" != xFalse; then
+    echo "$0: unexpected output: $output"
+    fail=1
+fi
+
+# With -b (and a server that supports it), meta context works.
+output=$(nbdkit -U - null --run 'nbdsh \
+    -b --connect "nbd+unix://?socket=$unixsocket" \
+    -c "print (h.can_meta_context (nbd.CONTEXT_BASE_ALLOCATION))"
</dev/null')
+if test "x$output" != xTrue; then
+    echo "$0: unexpected output: $output"
+    fail=1
+fi
+
+# Again, but with -b after --connect
+output=$(nbdkit -U - null --run 'nbdsh \
+    --connect "nbd+unix://?socket=$unixsocket" --base-allocation \
+    -c "print (h.can_meta_context (nbd.CONTEXT_BASE_ALLOCATION))"
</dev/null')
+if test "x$output" != xTrue; then
+    echo "$0: unexpected output: $output"
+    fail=1
+fi
+
+if [[ $(nbdkit --help) =~ --no-sr ]]; then
+    # meta context depends on server cooperation
+    output=$(nbdkit -U - --no-sr null --run 'nbdsh \
+      --connect "nbd+unix://?socket=$unixsocket" --base-allocation \
+      -c "print (h.can_meta_context (nbd.CONTEXT_BASE_ALLOCATION))"
</dev/null')
+    if test "x$output" != xFalse; then
+        echo "$0: unexpected output: $output"
+        fail=1
+    fi
+else
+    echo "$0: nbdkit lacks --no-sr"
+fi
+
+exit $fail
-- 
2.21.0

On Thu, Sep 12, 2019 at 01:54:20PM -0500, Eric Blake
wrote:
> We decided to not request the "base:allocation" context by
default (if
> a client wants to use block_status on a different context, then they'd
> have to get any default request out of the way); however, block status
> is useless without at least one meta context.  This adds a convenience
> knob for requesting that, and has the nice benefit of working with the
> --connect command line option (previously, if you wanted to use
> block_status, you could not use --connect, because requesting the meta
> context must happen before the connection).
There's no problem with the patch, but I wonder if it's a good idea to
burn though a single letter option (-b) which might be useful in
future for a more common purpose?  "buffer", "block",
"byte" all begin
with "b".

Even a two letter abbreviation is fine however (--ba).

Rich.
>  sh/nbdsh.pod       |  9 +++++++
>  python/nbdsh.py    |  4 +++
>  sh/Makefile.am     |  4 ++-
>  sh/test-context.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100755 sh/test-context.sh
> 
> diff --git a/sh/nbdsh.pod b/sh/nbdsh.pod
> index d7fc315..6c540c7 100644
> --- a/sh/nbdsh.pod
> +++ b/sh/nbdsh.pod
> @@ -56,6 +56,15 @@ __EXAMPLES_HEXDUMP__
> 
>  Display brief command line help and exit.
> 
> +=item B<-b>
> +
> +=item B<--base-allocation>
> +
> +Request the use of the "base:allocation" meta context, which is
the
> +most common context used with L<nbd_block_status(3)>.  This is
> +equivalent to calling S<C<h.set_meta_context
> +(nbd.CONTEXT_BASE_ALLOCATION)>> in the shell prior to connecting.
> +
>  =item B<-c> 'COMMAND ...'
> 
>  =item B<--command> 'COMMAND ...'
> diff --git a/python/nbdsh.py b/python/nbdsh.py
> index 319b0f0..00bc6bc 100644
> --- a/python/nbdsh.py
> +++ b/python/nbdsh.py
> @@ -27,6 +27,8 @@ def shell():
>      epilog = '''Please read the nbdsh(1) manual page for full
usage.'''
>      parser = argparse.ArgumentParser (prog='nbdsh',
description=description,
>                                        epilog=epilog)
> +    parser.add_argument ('-b', '--base-allocation',
action='store_true',
> +                         help='request the "base:allocation"
meta context')
>      parser.add_argument ('--connect', metavar='URI',
>                           help="connect to NBD URI")
>      parser.add_argument ('-c', '--command',
action='append',
> @@ -52,6 +54,8 @@ exit() or Ctrl-D                    # Quit the shell
>  help (nbd)                          # Display documentation
>  '''
> 
> +    if args.base_allocation:
> +        h.add_meta_context (nbd.CONTEXT_BASE_ALLOCATION)
>      if args.connect is not None:
>          h.connect_uri (args.connect)
>      # If there are no -c or --command parameters, go interactive,
> diff --git a/sh/Makefile.am b/sh/Makefile.am
> index 8f70e69..415e241 100644
> --- a/sh/Makefile.am
> +++ b/sh/Makefile.am
> @@ -21,6 +21,7 @@ EXTRA_DIST = \
>  	nbdsh.pod \
>  	examples/LICENSE-FOR-EXAMPLES \
>  	examples/hexdump.sh \
> +	test-context.sh \
>  	test-help.sh \
>  	test-pattern.sh \
>  	test-version.sh \
> @@ -43,7 +44,7 @@ nbdsh.1: nbdsh.pod $(top_builddir)/podwrapper.pl
> 
>  endif HAVE_POD
> 
> -TESTS_ENVIRONMENT = LIBNBD_DEBUG=1 EXP_VERSION=$(VERSION)
> +TESTS_ENVIRONMENT = LIBNBD_DEBUG=1 NBDKIT_DEBUG=1 EXP_VERSION=$(VERSION)
>  LOG_COMPILER = $(top_builddir)/run
>  TESTS = \
>  	test-help.sh \
> @@ -53,6 +54,7 @@ TESTS = \
>  if HAVE_NBDKIT
> 
>  TESTS += \
> +	test-context.sh \
>  	test-pattern.sh \
>  	$(NULL)
> 
> diff --git a/sh/test-context.sh b/sh/test-context.sh
> new file mode 100755
> index 0000000..55f9b53
> --- /dev/null
> +++ b/sh/test-context.sh
> @@ -0,0 +1,62 @@
> +#!/usr/bin/env bash
> +# nbd client library in userspace
> +# Copyright (C) 2019 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
> +# License as published by the Free Software Foundation; either
> +# version 2 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA
> +
> +# Test effects of nbdsh -b
> +fail=0
> +
> +# Without -b, no meta context is requested
> +output=$(nbdkit -U - null --run 'nbdsh \
> +    --connect "nbd+unix://?socket=$unixsocket" \
> +    -c "print (h.can_meta_context
(nbd.CONTEXT_BASE_ALLOCATION))" </dev/null')
> +if test "x$output" != xFalse; then
> +    echo "$0: unexpected output: $output"
> +    fail=1
> +fi
> +
> +# With -b (and a server that supports it), meta context works.
> +output=$(nbdkit -U - null --run 'nbdsh \
> +    -b --connect "nbd+unix://?socket=$unixsocket" \
> +    -c "print (h.can_meta_context
(nbd.CONTEXT_BASE_ALLOCATION))" </dev/null')
> +if test "x$output" != xTrue; then
> +    echo "$0: unexpected output: $output"
> +    fail=1
> +fi
> +
> +# Again, but with -b after --connect
> +output=$(nbdkit -U - null --run 'nbdsh \
> +    --connect "nbd+unix://?socket=$unixsocket" --base-allocation
\
> +    -c "print (h.can_meta_context
(nbd.CONTEXT_BASE_ALLOCATION))" </dev/null')
> +if test "x$output" != xTrue; then
> +    echo "$0: unexpected output: $output"
> +    fail=1
> +fi
> +
> +if [[ $(nbdkit --help) =~ --no-sr ]]; then
> +    # meta context depends on server cooperation
> +    output=$(nbdkit -U - --no-sr null --run 'nbdsh \
> +      --connect "nbd+unix://?socket=$unixsocket"
--base-allocation \
> +      -c "print (h.can_meta_context
(nbd.CONTEXT_BASE_ALLOCATION))" </dev/null')
> +    if test "x$output" != xFalse; then
> +        echo "$0: unexpected output: $output"
> +        fail=1
> +    fi
> +else
> +    echo "$0: nbdkit lacks --no-sr"
> +fi
> +
> +exit $fail
> -- 
> 2.21.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
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 9/12/19 1:54 PM, Eric Blake wrote:
> We decided to not request the "base:allocation" context by
default (if
> a client wants to use block_status on a different context, then they'd
> have to get any default request out of the way); however, block status
> is useless without at least one meta context.  This adds a convenience
> knob for requesting that, and has the nice benefit of working with the
> --connect command line option (previously, if you wanted to use
> block_status, you could not use --connect, because requesting the meta
> context must happen before the connection).
> ---
>  sh/nbdsh.pod       |  9 +++++++
>  python/nbdsh.py    |  4 +++
>  sh/Makefile.am     |  4 ++-
>  sh/test-context.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100755 sh/test-context.sh
> 
> diff --git a/sh/nbdsh.pod b/sh/nbdsh.pod
> index d7fc315..6c540c7 100644
> --- a/sh/nbdsh.pod
> +++ b/sh/nbdsh.pod
> @@ -56,6 +56,15 @@ __EXAMPLES_HEXDUMP__
> 
>  Display brief command line help and exit.
> 
> +=item B<-b>
Burning -b may be premature (we may want a --block-size parameter, for
example).  Similarly, -a (allocation, vs. append) or -m (meta context,
vs. memory) are not necessarily good letters.

If I just stick with long options for now, note that Python accepts --b
(or --ba) as unambiguously short for --base-allocation (at least as long
as there are no other --b... options added); we could also add a -b-a
synonym.

Also, we may want to revisit our decision to have no meta contexts
requested by default. Requesting "base:allocation" by default, but
allowing clients a way to unrequest that (in order request a different
context by itself, rather than appending a second context to our
default) might be an interesting idea.  In general, a client that is not
going to call nbd_block_status is going to start slightly faster if we
don't bother negotiating contexts, but at the same time, is not going to
be negatively impacted during the bulk of the runtime if we defaulted to
negotiating base:allocation.  If we _do_ default to negotiating
base:allocation, then we don't need an option at all.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org