[Libguestfs] [PATCH nbdkit] python: Implement can_extents + extents.

[Richard W.M. Jones]

On Mon, Aug 10, 2020 at 08:07:37AM -0500, Eric Blake wrote:
> >+      py_offset = PyTuple_GetItem (t, 0);
> >+      py_length = PyTuple_GetItem (t, 1);
> >+      py_type = PyTuple_GetItem (t, 2);
> >+      extent_offset = PyLong_AsUnsignedLongLong (py_offset);
> >+      extent_length = PyLong_AsUnsignedLongLong (py_length);
> >+      extent_type = PyLong_AsUnsignedLong (py_type);
> >+      if (check_python_failure ("PyLong") == -1) {
> 
> Is it really right to be doing error checking only once, but after
> three calls into PyLong_* functions?  I'm thinking of someone
> returning ('a',1,1).  But as long as the later successful calls
> don't wipe out an earlier failure, this looks like it works.

Yes, the error is thread-local and only cleared by calling
PyErr_Clear().

I modified the test to return ('a', 1, 1) element and it failed:

  nbdkit: python: error: test-python-plugin.py: PyLong: error: an integer is required

> >+        Py_DECREF (r);
> >+        return -1;
> >+      }
> >+      if (nbdkit_add_extent (extents,
> >+                             extent_offset, extent_length, extent_type) == -1) {
> >+        Py_DECREF (r);
> >+        return -1;
> >+      }
> >+    }
> >+
> >+    Py_DECREF (r);
> >+  }
> >+  else {
> >+    nbdkit_error ("%s not implemented", "extents");
> >+    return -1;
> 
> Here, we may be better off doing the same fallback as the C code,
> and just reporting the entire image as a single data extent, rather
> than always failing.

Yes, good idea, that's an easy change to make.

> >+++ b/tests/test_python.py
> 
> >@@ -220,3 +232,60 @@ class Test (unittest.TestCase):
> >          """Test cache."""
> >          self.connect ({"size": 512, "can_cache": "native"})
> >          self.h.cache (512, 0)
> >+
> >+    # We don't have access to the magic constants defined in the
> >+    # nbdkit module, so redefine them here.
> >+    EXTENT_HOLE = 1
> >+    EXTENT_ZERO = 2
> 
> ...these constants should have been available through 'import nbdkit'.

Not in this test, because nbdkit isn't a real module, it's one
synthesized in python.c.  Perhaps it _should_ be, but that's not how
it's currently implemented.

> Otherwise this looks reasonable.

Thanks,

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