Libguestfs - Apr 2018 - [nbdkit PATCH 0/3] Test zero callback of python plugin

I'm planning on tweaking the language callbacks to support fua;
first up is the python bindings.  I want to move from:

def zero(h, count, offset, may_trim):

to a nicer:

def zero(h, count, offset, may_trim=False, fua=False):

where the C code passes keywords for the flags (we can add new
flags as needed), perhaps by using introspection to learn
whether the plugin has a mandatory may_trim (old-style) or
optional keywords (new style).  But any such changes FIRST need
to make sure I don't cause regressions, so beef up the testsuite
to cover the zero call in the first place :)

Eric Blake (3):
  python: Follow pep8 recommendations
  tests: Add coverage of zero in language bindings
  python: Test implementation of zero callback

 plugins/python/example.py | 10 ++++++++--
 tests/python-exception.py |  4 ++++
 tests/shebang.py          |  5 ++++-
 tests/test-lang-plugins.c |  7 +++++++
 tests/test.py             | 22 +++++++++++++++++++---
 5 files changed, 42 insertions(+), 6 deletions(-)

-- 
2.14.3

No semantic change, just fixing style issues to comply with
pep8 rules.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/example.py | 10 ++++++++--
 tests/python-exception.py |  4 ++++
 tests/shebang.py          |  5 ++++-
 tests/test.py             | 17 ++++++++++++++---
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/plugins/python/example.py b/plugins/python/example.py
index be9d9a8..b0f2185 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -33,11 +33,13 @@ import errno
 # per handle.
 disk = bytearray(1024 * 1024)

+
 # This just prints the extra command line parameters, but real plugins
 # should parse them and reject any unknown parameters.
 def config(key, value):
     print ("ignored parameter %s=%s" % (key, value))

+
 def open(readonly):
     print ("open: readonly=%d" % readonly)

@@ -46,19 +48,23 @@ def open(readonly):
     # callbacks [in the client connected phase].
     return 1

+
 def get_size(h):
     global disk
-    return len (disk)
+    return len(disk)
+

 def pread(h, count, offset):
     global disk
     return disk[offset:offset+count]

+
 def pwrite(h, buf, offset):
     global disk
-    end = offset + len (buf)
+    end = offset + len(buf)
     disk[offset:end] = buf

+
 def zero(h, count, offset, may_trim):
     global disk
     if may_trim:
diff --git a/tests/python-exception.py b/tests/python-exception.py
index 93531cc..1debf51 100644
--- a/tests/python-exception.py
+++ b/tests/python-exception.py
@@ -32,14 +32,18 @@

 # A dummy python plugin which just raises an exception in config_complete.

+
 def config_complete():
     raise RuntimeError("this is the test string")

+
 def open(readonly):
     return 1

+
 def get_size(h):
     return 0

+
 def pread(h, count, offset):
     return ""
diff --git a/tests/shebang.py b/tests/shebang.py
index 012a73e..c7d833e 100755
--- a/tests/shebang.py
+++ b/tests/shebang.py
@@ -2,13 +2,16 @@

 disk = bytearray(1024 * 1024)

+
 def open(readonly):
     print ("open: readonly=%d" % readonly)
     return 1

+
 def get_size(h):
     global disk
-    return len (disk)
+    return len(disk)
+

 def pread(h, count, offset):
     global disk
diff --git a/tests/test.py b/tests/test.py
index 5d68b32..015b20f 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -1,38 +1,49 @@
-disk = bytearray (1024*1024);
+disk = bytearray(1024*1024)
+

 def config_complete():
     pass

+
 def open(readonly):
     return 1

+
 def get_size(h):
     global disk
-    return len (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):
     global disk
     return disk[offset:offset+count]

+
 def pwrite(h, buf, offset):
     global disk
-    end = offset + len (buf)
+    end = offset + len(buf)
     disk[offset:end] = buf

+
 def flush(h):
     pass

+
 def trim(h, count, offset):
     pass
-- 
2.14.3

The existing test of language bindings was not covering any
use of NBD_CMD_WRITE_ZEROES, making it harder to test changes
to the plugin's zero callback.  Recent Linux kernels are now
smart enough to turn fallocate(2) with FALLOC_FL_ZERO_RANGE
into a SCSI WRITE SAME request, which qemu in turn converts
into an NBD_CMD_WRITE_ZEROES.  Since libguestfs does not
directly have an API for this, we have to use the backdoor
of guestfs_debug and fallocate(1).  But inspecting logs such
as tests/test-python.log, this change DOES prove that we
got a client request for write zeroes, and that the fallback
to pwrite due to test.py lacking a zero callback works.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-lang-plugins.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/test-lang-plugins.c b/tests/test-lang-plugins.c
index b695aaf..17e757b 100644
--- a/tests/test-lang-plugins.c
+++ b/tests/test-lang-plugins.c
@@ -122,6 +122,13 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
 #endif

+  /* Run fallocate(1) on the device to test zero path. */
+  if (guestfs_umount (g, "/") == 01)
+    exit (EXIT_FAILURE);
+  const char *cmd[] = { "fallocate", "-nzl",
"64k", "/dev/sda", NULL };
+  char *s = guestfs_debug (g, "sh", (char **) cmd);
+  free (s);
+
   if (guestfs_shutdown (g) == -1)
     exit (EXIT_FAILURE);

-- 
2.14.3

The previous commit enabled testing of the fallback when a
zero callback is not present; but even better is testing
that the zero callback is called correctly.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/test.py b/tests/test.py
index 015b20f..630ac2f 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -41,6 +41,11 @@ def pwrite(h, buf, offset):
     disk[offset:end] = buf


+def zero(h, count, offset, may_trim=False):
+    global disk
+    disk[offset:offset+count] = bytearray(count)
+
+
 def flush(h):
     pass

-- 
2.14.3

On 04/05/2018 01:54 PM, Eric Blake wrote:
> The existing test of language bindings was not covering any
> use of NBD_CMD_WRITE_ZEROES, making it harder to test changes
> to the plugin's zero callback.  Recent Linux kernels are now
> smart enough to turn fallocate(2) with FALLOC_FL_ZERO_RANGE
> into a SCSI WRITE SAME request, which qemu in turn converts
> into an NBD_CMD_WRITE_ZEROES.  Since libguestfs does not
> directly have an API for this, we have to use the backdoor
> of guestfs_debug and fallocate(1).  But inspecting logs such
> as tests/test-python.log, this change DOES prove that we
> got a client request for write zeroes, and that the fallback
> to pwrite due to test.py lacking a zero callback works.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-lang-plugins.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tests/test-lang-plugins.c b/tests/test-lang-plugins.c
> index b695aaf..17e757b 100644
> --- a/tests/test-lang-plugins.c
> +++ b/tests/test-lang-plugins.c
> @@ -122,6 +122,13 @@ main (int argc, char *argv[])
>      exit (EXIT_FAILURE);
>  #endif
> 
> +  /* Run fallocate(1) on the device to test zero path. */
> +  if (guestfs_umount (g, "/") == 01)
That should be -1.
> +    exit (EXIT_FAILURE);
> +  const char *cmd[] = { "fallocate", "-nzl",
"64k", "/dev/sda", NULL };
> +  char *s = guestfs_debug (g, "sh", (char **) cmd);
> +  free (s);
> +
>    if (guestfs_shutdown (g) == -1)
>      exit (EXIT_FAILURE);
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Thu, Apr 05, 2018 at 01:54:36PM -0500, Eric Blake
wrote:
> The previous commit enabled testing of the fallback when a
> zero callback is not present; but even better is testing
> that the zero callback is called correctly.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tests/test.py b/tests/test.py
> index 015b20f..630ac2f 100644
> --- a/tests/test.py
> +++ b/tests/test.py
> @@ -41,6 +41,11 @@ def pwrite(h, buf, offset):
>      disk[offset:end] = buf
> 
> 
> +def zero(h, count, offset, may_trim=False):
> +    global disk
> +    disk[offset:offset+count] = bytearray(count)
> +
> +
>  def flush(h):
>      pass
> 
ACK series (with the obvious 01 => 1 change).

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