Libguestfs - Nov 2019 - Re: [PATCH nbdkit v2 10/10] tests: Test the Python plugin thoroughly.

On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> This tests the Python plugin thoroughly by issuing client commands
> through libnbd and checking we get the expected results.
> ---
>  tests/Makefile.am           |  13 +--
>  tests/test-python-plugin.py | 134 ++++++++++++++++++++++++++++
>  tests/test-python.py        | 172 ++++++++++++++++++++++++++++++++++++
>  tests/test.py               |  60 -------------
>  4 files changed, 309 insertions(+), 70 deletions(-)
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 13cd8f3..88849f5 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -115,7 +115,8 @@ EXTRA_DIST = \
>         test-pattern-largest-for-qemu.sh \
>         test-python-exception.sh \
>         test.pl \
> -       test.py \
> +       test-python.py \
> +       test-python-plugin.py \
>         test-rate.sh \
>         test-rate-dynamic.sh \
>         test.rb \
> @@ -787,18 +788,10 @@ endif HAVE_PERL
>  if HAVE_PYTHON
>
>  TESTS += \
> +       test-python.py \
>         test-python-exception.sh \
>         test-shebang-python.sh \
>         $(NULL)
> -LIBGUESTFS_TESTS += test-python
> -
> -test_python_SOURCES = test-lang-plugins.c test.h
> -test_python_CFLAGS = \
> -       -DLANG='"python"'
-DSCRIPT='"$(srcdir)/test.py"' \
> -       $(WARNINGS_CFLAGS) \
> -       $(LIBGUESTFS_CFLAGS) \
> -       $(NULL)
> -test_python_LDADD = libtest.la $(LIBGUESTFS_LIBS)
>
>  endif HAVE_PYTHON
>
> diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py
> new file mode 100644
> index 0000000..053a380
> --- /dev/null
> +++ b/tests/test-python-plugin.py
> @@ -0,0 +1,134 @@
> +#!/usr/bin/env python3
> +# nbdkit
> +# Copyright (C) 2019 Red Hat Inc.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +# See test-python.py.
> +
> +import nbdkit
> +import sys
> +import pickle
> +import codecs
> +
> +def api_version ():
> +    return 2
> +
> +cfg = {}
> +
> +def config (k, v):
> +    global cfg
> +    if k == "cfg":
> +        cfg = pickle.loads (codecs.decode (v.encode(),
"base64"))
> +
> +def config_complete ():
> +    print ("set_error = %r" % nbdkit.set_error)
> +
> +def open (readonly):
> +    return {
> +        'disk': bytearray (cfg.get ('size', 0))
> +    }
> +
> +def get_size (h):
> +    return len (h['disk'])
> +
> +def is_rotational(h):
> +    return cfg.get ('is_rotational', False)
> +
> +def can_multi_conn (h):
> +    return cfg.get ('can_multi_conn', False)
> +
> +def can_write (h):
> +    return cfg.get ('can_write', True)
> +
> +def can_flush(h):
> +    return cfg.get ('can_flush', False)
> +
> +def can_trim(h):
> +    return cfg.get ('can_trim', False)
> +
> +def can_zero(h):
> +    return cfg.get ('can_zero', False)
> +
> +def can_fast_zero(h):
> +    return cfg.get ('can_fast_zero', False)
> +
> +def can_fua(h):
> +    fua = cfg.get ('can_fua', "none")
> +    if fua == "none":
> +        return nbdkit.FUA_NONE
> +    elif fua == "emulate":
> +        return nbdkit.FUA_EMULATE
> +    elif fua == "native":
> +        return nbdkit.FUA_NATIVE
> +
> +def can_cache(h):
> +    cache = cfg.get ('can_cache', "none")
> +    if cache == "none":
> +        return nbdkit.CACHE_NONE
> +    elif cache == "emulate":
> +        return nbdkit.CACHE_EMULATE
> +    elif cache == "native":
> +        return nbdkit.CACHE_NATIVE
> +
> +def pread(h, count, offset, flags):
> +    assert flags == 0
> +    return h['disk'][offset:offset+count]
Very nice and simple test plugin!

But this returns always a bytearray, which is also what nbdkit python plugin
expects. But real code using HTTPConnection return bytes:
>>> c = http.client.HTTPSConnection("www.python.org")
>>> c.request("GET", "/")
>>> r = c.getresponse()
>>> r.read()[:10]
b'<!doctype '

I think the plugin should support both bytearray, memoryview, or
bytes. Supporting objects
implementing the buffer protocol would be best.
> +
> +def pwrite(h, buf, offset, flags):
> +    expect_fua = cfg.get ('pwrite_expect_fua', False)
> +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> +    assert expect_fua == actual_fua
> +    end = offset + len(buf)
> +    h['disk'][offset:end] = buf
> +
> +def flush(h, flags):
> +    assert flags == 0
> +
> +def trim(h, count, offset, flags):
> +    expect_fua = cfg.get ('trim_expect_fua', False)
> +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> +    assert expect_fua == actual_fua
> +    h['disk'][offset:offset+count] = bytearray(count)
> +
> +def zero(h, count, offset, flags):
> +    expect_fua = cfg.get ('zero_expect_fua', False)
> +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> +    assert expect_fua == actual_fua
> +    expect_may_trim = cfg.get ('zero_expect_may_trim', False)
> +    actual_may_trim = bool (flags & nbdkit.FLAG_MAY_TRIM)
> +    assert expect_may_trim == actual_may_trim
> +    expect_fast_zero = cfg.get ('zero_expect_fast_zero', False)
> +    actual_fast_zero = bool (flags & nbdkit.FLAG_FAST_ZERO)
> +    assert expect_fast_zero == actual_fast_zero
> +    h['disk'][offset:offset+count] = bytearray(count)
> +
> +def cache(h, count, offset, flags):
> +    assert flags == 0
> +    # do nothing
> diff --git a/tests/test-python.py b/tests/test-python.py
> new file mode 100755
> index 0000000..2f565ba
> --- /dev/null
> +++ b/tests/test-python.py
> @@ -0,0 +1,172 @@
> +#!/usr/bin/env python3
> +# nbdkit
> +# Copyright (C) 2019 Red Hat Inc.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +# This tests the Python plugin thoroughly by issuing client commands
> +# through libnbd and checking we get the expected results.  It uses an
> +# associated plugin (test-python-plugin.sh).
test-python-plugin.py

This text can use python docstring:

"""
This tests ...
"""
> +
> +import os
> +import sys
> +import pickle
> +import codecs
> +
> +# Python has proven very difficult to valgrind, therefore it is disabled.
> +if "NBDKIT_VALGRIND" in os.environ:
> +    print ("$s: skipping Python test under valgrind" % __file__)
> +    sys.exit (77)
> +
> +try:
> +    import nbd
> +except:
except ImportError:
> +    print ("%s: skipped because cannot import python libnbd" %
__file__)
> +    sys.exit (77)
> +
> +try:
> +    srcdir = os.environ['SRCDIR']
> +except:
except KeyError:
> +    print ("%s: FAIL: test failed because $SRCDIR is not set" %
__file__)
> +    sys.exit (1)
> +
> +def test (cfg):
test will fool testing frameworks, expecting that this is a test function.
Maybe call this handle()? or case()?
> +    h = nbd.NBD ()
> +    cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
base64.b64encode() is better, avoiding unwanted newlines.
> +    cmd = ["nbdkit", "-v", "-s",
"--exit-with-parent",
> +           "python", srcdir +
"/test-python-plugin.py",
> +           "cfg=" + cfg]
> +    h.connect_command (cmd)
> +    return h
> +
> +# Test we can send an empty pickled test configuration and do nothing
> +# else.  This is just to ensure the machinery of the test works.
> +h = test ({})
So we have now running nbdkit that will exit the python collects when h
is implicitly closed when creating a new handle?

This is fragile, but can be solved with the help of a testing framework.
> +
> +# Test the size.
> +h = test ({"size": 512})
> +assert h.get_size() == 512
> +h = test ({"size": 1024*1024})
> +assert h.get_size() == 1024*1024
These tests will fail when on the first error, which is less useful.

With very little work we can convert this to pytest:

def test_size():
   h = test()
   assert h.get_size() == 512

To run the test you use:

pytest test-python.py
> +
> +# Test each flag call.
> +h = test ({"size": 512, "is_rotational": True})
> +assert h.is_rotational()
> +h = test ({"size": 512, "is_rotational": False})
> +assert not h.is_rotational()
When this fails, you will get unhelpful output:

    assert not True

But with pytest you get much more useful error, something like:

    def test_is_rotational():
        h = handle ({"size": 512, "is_rotational":
True})
>       assert not h.is_rotational
E       assert not True
E        +  where True = <test.handle.<locals>.H object at
0x7faa3358e450>.is_rotational

(I faked the handle object for this example)
> +
> +h = test ({"size": 512, "can_multi_conn": True})
> +assert h.can_multi_conn()
> +h = test ({"size": 512, "can_multi_conn": False})
> +assert not h.can_multi_conn()
> +
> +h = test ({"size": 512, "can_write": True})
> +assert not h.is_read_only()
> +h = test ({"size": 512, "can_write": False})
> +assert h.is_read_only()
> +
> +h = test ({"size": 512, "can_flush": True})
> +assert h.can_flush()
> +h = test ({"size": 512, "can_flush": False})
> +assert not h.can_flush()
> +
> +h = test ({"size": 512, "can_trim": True})
> +assert h.can_trim()
> +h = test ({"size": 512, "can_trim": False})
> +assert not h.can_trim()
> +
> +# nbdkit can always zero because it emulates it.
> +#h = test ({"size": 512, "can_zero": True})
> +#assert h.can_zero()
> +#h = test ({"size": 512, "can_zero": False})
> +#assert not h.can_zero()
> +
> +h = test ({"size": 512, "can_fast_zero": True})
> +assert h.can_fast_zero()
> +h = test ({"size": 512, "can_fast_zero": False})
> +assert not h.can_fast_zero()
> +
> +h = test ({"size": 512, "can_fua": "none"})
> +assert not h.can_fua()
> +h = test ({"size": 512, "can_fua":
"emulate"})
> +assert h.can_fua()
> +h = test ({"size": 512, "can_fua":
"native"})
> +assert h.can_fua()
> +
> +h = test ({"size": 512, "can_cache":
"none"})
> +assert not h.can_cache()
> +h = test ({"size": 512, "can_cache":
"emulate"})
> +assert h.can_cache()
> +h = test ({"size": 512, "can_cache":
"native"})
> +assert h.can_cache()
> +
> +# Not yet implemented: can_extents.
> +
> +# Test pread.
> +h = test ({"size": 512})
> +buf = h.pread (512, 0)
> +
> +# Test pwrite + flags.
> +h = test ({"size": 512})
> +h.pwrite (buf, 0)
> +
> +h = test ({"size": 512, "can_fua": "native",
"pwrite_expect_fua": True})
> +h.pwrite (buf, 0, nbd.CMD_FLAG_FUA)
> +
> +# Test flush.
> +h = test ({"size": 512, "can_flush": True})
> +h.flush ()
> +
> +# Test trim + flags.
> +h = test ({"size": 512, "can_trim": True})
> +h.trim (512, 0)
> +
> +h = test ({"size": 512,
> +           "can_trim": True, "can_fua":
"native", "trim_expect_fua": True})
This becomes messy. With the dict does not fit in one line, or
contains more than
few keys it is more readable to use one key: value pair per line.
> +h.trim (512, 0, nbd.CMD_FLAG_FUA)
> +
> +# Test zero + flags.
> +h = test ({"size": 512, "can_zero": True})
> +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE)
> +
> +h = test ({"size": 512,
> +           "can_zero": True, "can_fua":
"native", "zero_expect_fua": True})
> +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FUA)
> +
> +h = test ({"size": 512, "can_zero": True,
"zero_expect_may_trim": True})
> +h.zero (512, 0, 0) # absence of nbd.CMD_FLAG_NO_HOLE
> +
> +h = test ({"size": 512,
> +           "can_zero": True, "can_fast_zero": True,
> +           "zero_expect_fast_zero": True})
> +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FAST_ZERO)
> +
> +# Test cache.
> +h = test ({"size": 512, "can_cache":
"native"})
> +h.cache (512, 0)
> diff --git a/tests/test.py b/tests/test.py
> deleted file mode 100644
> index ac80d96..0000000
> --- a/tests/test.py
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -import nbdkit
> -
> -disk = bytearray(1024*1024)
> -
> -
> -def api_version():
> -    return 2
> -
> -
> -def config_complete():
> -    print ("set_error = %r" % nbdkit.set_error)
> -
> -
> -def open(readonly):
> -    return 1
> -
> -
> -def get_size(h):
> -    global disk
> -    return len(disk)
> -
> -
> -def can_write(h):
> -    return True
> -
> -
> -def can_flush(h):
> -    return True
> -
> -
> -def is_rotational(h):
> -    return False
> -
> -
> -def can_trim(h):
> -    return True
> -
> -
> -def pread(h, count, offset, flags):
> -    global disk
> -    return disk[offset:offset+count]
> -
> -
> -def pwrite(h, buf, offset, flags):
> -    global disk
> -    end = offset + len(buf)
> -    disk[offset:end] = buf
> -
> -
> -def flush(h, flags):
> -    pass
> -
> -
> -def trim(h, count, offset, flags):
> -    pass
> -
> -
> -def zero(h, count, offset, flags):
> -    global disk
> -    disk[offset:offset+count] = bytearray(count)
> --
> 2.23.0
>
Otherwise very nice tests!

Nir

On Sat, Nov 23, 2019 at 1:42 AM Nir Soffer <nsoffer@redhat.com>
wrote:
>
> On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones
<rjones@redhat.com> wrote:
> >
> > This tests the Python plugin thoroughly by issuing client commands
> > through libnbd and checking we get the expected results.
> > ---
> >  tests/Makefile.am           |  13 +--
> >  tests/test-python-plugin.py | 134 ++++++++++++++++++++++++++++
> >  tests/test-python.py        | 172
++++++++++++++++++++++++++++++++++++
> >  tests/test.py               |  60 -------------
> >  4 files changed, 309 insertions(+), 70 deletions(-)
> >
> > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > index 13cd8f3..88849f5 100644
> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -115,7 +115,8 @@ EXTRA_DIST = \
> >         test-pattern-largest-for-qemu.sh \
> >         test-python-exception.sh \
> >         test.pl \
> > -       test.py \
> > +       test-python.py \
> > +       test-python-plugin.py \
> >         test-rate.sh \
> >         test-rate-dynamic.sh \
> >         test.rb \
> > @@ -787,18 +788,10 @@ endif HAVE_PERL
> >  if HAVE_PYTHON
> >
> >  TESTS += \
> > +       test-python.py \
> >         test-python-exception.sh \
> >         test-shebang-python.sh \
> >         $(NULL)
> > -LIBGUESTFS_TESTS += test-python
> > -
> > -test_python_SOURCES = test-lang-plugins.c test.h
> > -test_python_CFLAGS = \
> > -       -DLANG='"python"'
-DSCRIPT='"$(srcdir)/test.py"' \
> > -       $(WARNINGS_CFLAGS) \
> > -       $(LIBGUESTFS_CFLAGS) \
> > -       $(NULL)
> > -test_python_LDADD = libtest.la $(LIBGUESTFS_LIBS)
> >
> >  endif HAVE_PYTHON
> >
> > diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py
> > new file mode 100644
> > index 0000000..053a380
> > --- /dev/null
> > +++ b/tests/test-python-plugin.py
> > @@ -0,0 +1,134 @@
> > +#!/usr/bin/env python3
> > +# nbdkit
> > +# Copyright (C) 2019 Red Hat Inc.
> > +#
> > +# Redistribution and use in source and binary forms, with or without
> > +# modification, are permitted provided that the following conditions
are
> > +# met:
> > +#
> > +# * Redistributions of source code must retain the above copyright
> > +# notice, this list of conditions and the following disclaimer.
> > +#
> > +# * Redistributions in binary form must reproduce the above copyright
> > +# notice, this list of conditions and the following disclaimer in the
> > +# documentation and/or other materials provided with the
distribution.
> > +#
> > +# * Neither the name of Red Hat nor the names of its contributors may
be
> > +# used to endorse or promote products derived from this software
without
> > +# specific prior written permission.
> > +#
> > +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
> > +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> > +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> > +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> > +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> > +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> > +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> > +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > +# SUCH DAMAGE.
> > +
> > +# See test-python.py.
> > +
> > +import nbdkit
> > +import sys
> > +import pickle
> > +import codecs
> > +
> > +def api_version ():
> > +    return 2
> > +
> > +cfg = {}
> > +
> > +def config (k, v):
> > +    global cfg
> > +    if k == "cfg":
> > +        cfg = pickle.loads (codecs.decode (v.encode(),
"base64"))
> > +
> > +def config_complete ():
> > +    print ("set_error = %r" % nbdkit.set_error)
> > +
> > +def open (readonly):
> > +    return {
> > +        'disk': bytearray (cfg.get ('size', 0))
> > +    }
> > +
> > +def get_size (h):
> > +    return len (h['disk'])
> > +
> > +def is_rotational(h):
> > +    return cfg.get ('is_rotational', False)
> > +
> > +def can_multi_conn (h):
> > +    return cfg.get ('can_multi_conn', False)
> > +
> > +def can_write (h):
> > +    return cfg.get ('can_write', True)
> > +
> > +def can_flush(h):
> > +    return cfg.get ('can_flush', False)
> > +
> > +def can_trim(h):
> > +    return cfg.get ('can_trim', False)
> > +
> > +def can_zero(h):
> > +    return cfg.get ('can_zero', False)
> > +
> > +def can_fast_zero(h):
> > +    return cfg.get ('can_fast_zero', False)
> > +
> > +def can_fua(h):
> > +    fua = cfg.get ('can_fua', "none")
> > +    if fua == "none":
> > +        return nbdkit.FUA_NONE
> > +    elif fua == "emulate":
> > +        return nbdkit.FUA_EMULATE
> > +    elif fua == "native":
> > +        return nbdkit.FUA_NATIVE
> > +
> > +def can_cache(h):
> > +    cache = cfg.get ('can_cache', "none")
> > +    if cache == "none":
> > +        return nbdkit.CACHE_NONE
> > +    elif cache == "emulate":
> > +        return nbdkit.CACHE_EMULATE
> > +    elif cache == "native":
> > +        return nbdkit.CACHE_NATIVE
> > +
> > +def pread(h, count, offset, flags):
> > +    assert flags == 0
> > +    return h['disk'][offset:offset+count]
>
> Very nice and simple test plugin!
>
> But this returns always a bytearray, which is also what nbdkit python
plugin
> expects. But real code using HTTPConnection return bytes:
>
> >>> c = http.client.HTTPSConnection("www.python.org")
> >>> c.request("GET", "/")
> >>> r = c.getresponse()
> >>> r.read()[:10]
> b'<!doctype '
>
> I think the plugin should support both bytearray, memoryview, or
> bytes. Supporting objects
> implementing the buffer protocol would be best.
This patch adds support for buffer protocol:
https://www.redhat.com/archives/libguestfs/2019-November/msg00200.html

(I forgot to add nbdkit to the title)
>
> > +
> > +def pwrite(h, buf, offset, flags):
> > +    expect_fua = cfg.get ('pwrite_expect_fua', False)
> > +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> > +    assert expect_fua == actual_fua
> > +    end = offset + len(buf)
> > +    h['disk'][offset:end] = buf
> > +
> > +def flush(h, flags):
> > +    assert flags == 0
> > +
> > +def trim(h, count, offset, flags):
> > +    expect_fua = cfg.get ('trim_expect_fua', False)
> > +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> > +    assert expect_fua == actual_fua
> > +    h['disk'][offset:offset+count] = bytearray(count)
> > +
> > +def zero(h, count, offset, flags):
> > +    expect_fua = cfg.get ('zero_expect_fua', False)
> > +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> > +    assert expect_fua == actual_fua
> > +    expect_may_trim = cfg.get ('zero_expect_may_trim', False)
> > +    actual_may_trim = bool (flags & nbdkit.FLAG_MAY_TRIM)
> > +    assert expect_may_trim == actual_may_trim
> > +    expect_fast_zero = cfg.get ('zero_expect_fast_zero',
False)
> > +    actual_fast_zero = bool (flags & nbdkit.FLAG_FAST_ZERO)
> > +    assert expect_fast_zero == actual_fast_zero
> > +    h['disk'][offset:offset+count] = bytearray(count)
> > +
> > +def cache(h, count, offset, flags):
> > +    assert flags == 0
> > +    # do nothing
> > diff --git a/tests/test-python.py b/tests/test-python.py
> > new file mode 100755
> > index 0000000..2f565ba
> > --- /dev/null
> > +++ b/tests/test-python.py
> > @@ -0,0 +1,172 @@
> > +#!/usr/bin/env python3
> > +# nbdkit
> > +# Copyright (C) 2019 Red Hat Inc.
> > +#
> > +# Redistribution and use in source and binary forms, with or without
> > +# modification, are permitted provided that the following conditions
are
> > +# met:
> > +#
> > +# * Redistributions of source code must retain the above copyright
> > +# notice, this list of conditions and the following disclaimer.
> > +#
> > +# * Redistributions in binary form must reproduce the above copyright
> > +# notice, this list of conditions and the following disclaimer in the
> > +# documentation and/or other materials provided with the
distribution.
> > +#
> > +# * Neither the name of Red Hat nor the names of its contributors may
be
> > +# used to endorse or promote products derived from this software
without
> > +# specific prior written permission.
> > +#
> > +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
> > +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> > +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> > +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> > +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> > +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> > +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> > +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > +# SUCH DAMAGE.
> > +
> > +# This tests the Python plugin thoroughly by issuing client commands
> > +# through libnbd and checking we get the expected results.  It uses
an
> > +# associated plugin (test-python-plugin.sh).
>
> test-python-plugin.py
>
> This text can use python docstring:
>
> """
> This tests ...
> """
>
> > +
> > +import os
> > +import sys
> > +import pickle
> > +import codecs
> > +
> > +# Python has proven very difficult to valgrind, therefore it is
disabled.
> > +if "NBDKIT_VALGRIND" in os.environ:
> > +    print ("$s: skipping Python test under valgrind" %
__file__)
> > +    sys.exit (77)
> > +
> > +try:
> > +    import nbd
> > +except:
>
> except ImportError:
>
> > +    print ("%s: skipped because cannot import python
libnbd" % __file__)
> > +    sys.exit (77)
> > +
> > +try:
> > +    srcdir = os.environ['SRCDIR']
> > +except:
>
> except KeyError:
>
> > +    print ("%s: FAIL: test failed because $SRCDIR is not
set" % __file__)
> > +    sys.exit (1)
> > +
> > +def test (cfg):
>
> test will fool testing frameworks, expecting that this is a test function.
> Maybe call this handle()? or case()?
>
> > +    h = nbd.NBD ()
> > +    cfg = codecs.encode (pickle.dumps (cfg),
"base64").decode()
>
> base64.b64encode() is better, avoiding unwanted newlines.
>
> > +    cmd = ["nbdkit", "-v", "-s",
"--exit-with-parent",
> > +           "python", srcdir +
"/test-python-plugin.py",
> > +           "cfg=" + cfg]
> > +    h.connect_command (cmd)
> > +    return h
> > +
> > +# Test we can send an empty pickled test configuration and do nothing
> > +# else.  This is just to ensure the machinery of the test works.
> > +h = test ({})
>
> So we have now running nbdkit that will exit the python collects when h
> is implicitly closed when creating a new handle?
>
> This is fragile, but can be solved with the help of a testing framework.
>
> > +
> > +# Test the size.
> > +h = test ({"size": 512})
> > +assert h.get_size() == 512
> > +h = test ({"size": 1024*1024})
> > +assert h.get_size() == 1024*1024
>
> These tests will fail when on the first error, which is less useful.
>
> With very little work we can convert this to pytest:
>
> def test_size():
>    h = test()
>    assert h.get_size() == 512
>
> To run the test you use:
>
> pytest test-python.py
>
> > +
> > +# Test each flag call.
> > +h = test ({"size": 512, "is_rotational": True})
> > +assert h.is_rotational()
> > +h = test ({"size": 512, "is_rotational": False})
> > +assert not h.is_rotational()
>
> When this fails, you will get unhelpful output:
>
>     assert not True
>
> But with pytest you get much more useful error, something like:
>
>     def test_is_rotational():
>         h = handle ({"size": 512, "is_rotational":
True})
> >       assert not h.is_rotational
> E       assert not True
> E        +  where True = <test.handle.<locals>.H object at
> 0x7faa3358e450>.is_rotational
>
> (I faked the handle object for this example)
>
> > +
> > +h = test ({"size": 512, "can_multi_conn": True})
> > +assert h.can_multi_conn()
> > +h = test ({"size": 512, "can_multi_conn": False})
> > +assert not h.can_multi_conn()
> > +
> > +h = test ({"size": 512, "can_write": True})
> > +assert not h.is_read_only()
> > +h = test ({"size": 512, "can_write": False})
> > +assert h.is_read_only()
> > +
> > +h = test ({"size": 512, "can_flush": True})
> > +assert h.can_flush()
> > +h = test ({"size": 512, "can_flush": False})
> > +assert not h.can_flush()
> > +
> > +h = test ({"size": 512, "can_trim": True})
> > +assert h.can_trim()
> > +h = test ({"size": 512, "can_trim": False})
> > +assert not h.can_trim()
> > +
> > +# nbdkit can always zero because it emulates it.
> > +#h = test ({"size": 512, "can_zero": True})
> > +#assert h.can_zero()
> > +#h = test ({"size": 512, "can_zero": False})
> > +#assert not h.can_zero()
> > +
> > +h = test ({"size": 512, "can_fast_zero": True})
> > +assert h.can_fast_zero()
> > +h = test ({"size": 512, "can_fast_zero": False})
> > +assert not h.can_fast_zero()
> > +
> > +h = test ({"size": 512, "can_fua":
"none"})
> > +assert not h.can_fua()
> > +h = test ({"size": 512, "can_fua":
"emulate"})
> > +assert h.can_fua()
> > +h = test ({"size": 512, "can_fua":
"native"})
> > +assert h.can_fua()
> > +
> > +h = test ({"size": 512, "can_cache":
"none"})
> > +assert not h.can_cache()
> > +h = test ({"size": 512, "can_cache":
"emulate"})
> > +assert h.can_cache()
> > +h = test ({"size": 512, "can_cache":
"native"})
> > +assert h.can_cache()
> > +
> > +# Not yet implemented: can_extents.
> > +
> > +# Test pread.
> > +h = test ({"size": 512})
> > +buf = h.pread (512, 0)
> > +
> > +# Test pwrite + flags.
> > +h = test ({"size": 512})
> > +h.pwrite (buf, 0)
> > +
> > +h = test ({"size": 512, "can_fua":
"native", "pwrite_expect_fua": True})
> > +h.pwrite (buf, 0, nbd.CMD_FLAG_FUA)
> > +
> > +# Test flush.
> > +h = test ({"size": 512, "can_flush": True})
> > +h.flush ()
> > +
> > +# Test trim + flags.
> > +h = test ({"size": 512, "can_trim": True})
> > +h.trim (512, 0)
> > +
> > +h = test ({"size": 512,
> > +           "can_trim": True, "can_fua":
"native", "trim_expect_fua": True})
>
> This becomes messy. With the dict does not fit in one line, or
> contains more than
> few keys it is more readable to use one key: value pair per line.
>
> > +h.trim (512, 0, nbd.CMD_FLAG_FUA)
> > +
> > +# Test zero + flags.
> > +h = test ({"size": 512, "can_zero": True})
> > +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE)
> > +
> > +h = test ({"size": 512,
> > +           "can_zero": True, "can_fua":
"native", "zero_expect_fua": True})
> > +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FUA)
> > +
> > +h = test ({"size": 512, "can_zero": True,
"zero_expect_may_trim": True})
> > +h.zero (512, 0, 0) # absence of nbd.CMD_FLAG_NO_HOLE
> > +
> > +h = test ({"size": 512,
> > +           "can_zero": True, "can_fast_zero":
True,
> > +           "zero_expect_fast_zero": True})
> > +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FAST_ZERO)
> > +
> > +# Test cache.
> > +h = test ({"size": 512, "can_cache":
"native"})
> > +h.cache (512, 0)
> > diff --git a/tests/test.py b/tests/test.py
> > deleted file mode 100644
> > index ac80d96..0000000
> > --- a/tests/test.py
> > +++ /dev/null
> > @@ -1,60 +0,0 @@
> > -import nbdkit
> > -
> > -disk = bytearray(1024*1024)
> > -
> > -
> > -def api_version():
> > -    return 2
> > -
> > -
> > -def config_complete():
> > -    print ("set_error = %r" % nbdkit.set_error)
> > -
> > -
> > -def open(readonly):
> > -    return 1
> > -
> > -
> > -def get_size(h):
> > -    global disk
> > -    return len(disk)
> > -
> > -
> > -def can_write(h):
> > -    return True
> > -
> > -
> > -def can_flush(h):
> > -    return True
> > -
> > -
> > -def is_rotational(h):
> > -    return False
> > -
> > -
> > -def can_trim(h):
> > -    return True
> > -
> > -
> > -def pread(h, count, offset, flags):
> > -    global disk
> > -    return disk[offset:offset+count]
> > -
> > -
> > -def pwrite(h, buf, offset, flags):
> > -    global disk
> > -    end = offset + len(buf)
> > -    disk[offset:end] = buf
> > -
> > -
> > -def flush(h, flags):
> > -    pass
> > -
> > -
> > -def trim(h, count, offset, flags):
> > -    pass
> > -
> > -
> > -def zero(h, count, offset, flags):
> > -    global disk
> > -    disk[offset:offset+count] = bytearray(count)
> > --
> > 2.23.0
> >
>
> Otherwise very nice tests!
>
> Nir

On Sat, Nov 23, 2019 at 01:42:15AM +0200, Nir Soffer
wrote:
> On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones
<rjones@redhat.com> wrote:
> > +def pread(h, count, offset, flags):
> > +    assert flags == 0
> > +    return h['disk'][offset:offset+count]
> 
> Very nice and simple test plugin!
> 
> But this returns always a bytearray, which is also what nbdkit python
plugin
> expects. But real code using HTTPConnection return bytes:
> 
> >>> c = http.client.HTTPSConnection("www.python.org")
> >>> c.request("GET", "/")
> >>> r = c.getresponse()
> >>> r.read()[:10]
> b'<!doctype '
> 
> I think the plugin should support both bytearray, memoryview, or
> bytes. Supporting objects
> implementing the buffer protocol would be best.
I thought bytearray & bytes were the same thing :-/

Would adding this break existing nbdkit Python scripts?  Should this
be considered for v2 of the API?  Are there performance implications /
improvements from doing this?
> test-python-plugin.py
> 
> This text can use python docstring:
> 
> """
> This tests ...
> """
Good point, I'll fix this.  We could probably also have nbdkit do
something with the docstring, such as printing it in --help output,
although that's something for another patch series.
> > +    h = nbd.NBD ()
> > +    cfg = codecs.encode (pickle.dumps (cfg),
"base64").decode()
> 
> base64.b64encode() is better, avoiding unwanted newlines.
Ah OK, I originally added strip(), but this is better.
> > +    cmd = ["nbdkit", "-v", "-s",
"--exit-with-parent",
> > +           "python", srcdir +
"/test-python-plugin.py",
> > +           "cfg=" + cfg]
> > +    h.connect_command (cmd)
> > +    return h
> > +
> > +# Test we can send an empty pickled test configuration and do nothing
> > +# else.  This is just to ensure the machinery of the test works.
> > +h = test ({})
> 
> So we have now running nbdkit that will exit the python collects when h
> is implicitly closed when creating a new handle?
> 
> This is fragile, but can be solved with the help of a testing framework.
[...]
> pytest test-python.py
I'll probably use unittest though because it's built into Python and
because it's what we use in libguestfs, hivex etc but yes good idea.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Sat, Nov 23, 2019 at 3:10 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> On Sat, Nov 23, 2019 at 01:42:15AM +0200, Nir Soffer wrote:
> > On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones
<rjones@redhat.com> wrote:
> > > +def pread(h, count, offset, flags):
> > > +    assert flags == 0
> > > +    return h['disk'][offset:offset+count]
> >
> > Very nice and simple test plugin!
> >
> > But this returns always a bytearray, which is also what nbdkit python
plugin
> > expects. But real code using HTTPConnection return bytes:
> >
> > >>> c =
http.client.HTTPSConnection("www.python.org")
> > >>> c.request("GET", "/")
> > >>> r = c.getresponse()
> > >>> r.read()[:10]
> > b'<!doctype '
> >
> > I think the plugin should support both bytearray, memoryview, or
> > bytes. Supporting objects
> > implementing the buffer protocol would be best.
>
> I thought bytearray & bytes were the same thing :-/
>
> Would adding this break existing nbdkit Python scripts?  Should this
> be considered for v2 of the API?  Are there performance implications /
> improvements from doing this?
There is performance implication for v1 plugins like rhv-upload-plugin that
need to copy the bytes from imageio server on python side:

    r = http.getresponse()
    ...
    return bytearray(r.read())

This is sad because on the C side we mempcpy the data again. So with this patch
we avoid one copy of the two.

To avoid all unneeded copies, we need to change pread() to:

    def pread(h, buf, offset):

So the python side we can do:

    f.readinto(buf)

Or:

    sock.recv_info(buf)

It does not work for HTTPResponse, so in this case we have to do:

    buf[:] = r.read()

Since we work on v2 now, I think we should consider this change.

An uglier alternative is:

    def preadinto(h, buf, offset):

Matching python read() and readinto() interface.
> > test-python-plugin.py
> >
> > This text can use python docstring:
> >
> > """
> > This tests ...
> > """
>
> Good point, I'll fix this.  We could probably also have nbdkit do
> something with the docstring, such as printing it in --help output,
> although that's something for another patch series.
>
> > > +    h = nbd.NBD ()
> > > +    cfg = codecs.encode (pickle.dumps (cfg),
"base64").decode()
> >
> > base64.b64encode() is better, avoiding unwanted newlines.
>
> Ah OK, I originally added strip(), but this is better.
>
> > > +    cmd = ["nbdkit", "-v", "-s",
"--exit-with-parent",
> > > +           "python", srcdir +
"/test-python-plugin.py",
> > > +           "cfg=" + cfg]
> > > +    h.connect_command (cmd)
> > > +    return h
> > > +
> > > +# Test we can send an empty pickled test configuration and do
nothing
> > > +# else.  This is just to ensure the machinery of the test works.
> > > +h = test ({})
> >
> > So we have now running nbdkit that will exit the python collects when
h
> > is implicitly closed when creating a new handle?
> >
> > This is fragile, but can be solved with the help of a testing
framework.
> [...]
> > pytest test-python.py
>
> I'll probably use unittest though because it's built into Python
and
> because it's what we use in libguestfs, hivex etc but yes good idea.
>
> Thanks,
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> Fedora Windows cross-compiler. Compile Windows programs, test, and
> build Windows installers. Over 100 libraries supported.
> http://fedoraproject.org/wiki/MinGW
>