[Libguestfs] [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.

[Richard W.M. Jones]

On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
> On 11/22/19 1:53 PM, Richard W.M. Jones wrote:
> >To avoid breaking existing plugins, Python plugins wishing to use
> >version 2 of the API must opt in by declaring:
> >
> >   def api_version():
> >     return 2
> >
> >(Plugins which do not do this are assumed to want API version 1).
> 
> Could we also permit the python code to declare a global variable
> instead of a function?  But a function is just fine (and easier to
> integrate the way we do all our other callbacks).

I couldn't work out how to do this, plus we have the callback_defined
function which makes this easy, so yes, I did the easy thing :-)

> >@@ -54,20 +61,20 @@ def get_size(h):
> >      return len(disk)
> >-def pread(h, count, offset):
> >+def pread(h, count, offset, flags):
> >      global disk
> >      return disk[offset:offset+count]
> 
> Do we really want to be passing 'flags' as an integer that the
> python script must decode?

While I'm no Python programmer, it does look as if existing Python
core APIs are happy to use bitmasks, eg:

  https://docs.python.org/3/library/os.html#os.open

and this is also easy to implement and keeps it similar to the C API.

> >- def pwrite(h, buf, offset):
> >+ def pwrite(h, buf, offset, flags):
> >     length = len (buf)
> >     # no return value
> >  The body of your C<pwrite> function should write the C<buf> string to
> >  the disk.  You should write C<count> bytes to the disk starting at
> >-C<offset>.
> >+C<offset>.  C<flags> may contain C<nbdkit.FLAG_FUA>.
> 
> Should we mention that FLAG_FUA is only set if the python callback
> can_fua returned 1?  Or is that somewhat redundant with the fact
> that we already document that someone writing a python plugin should
> be familiar with the docs for C plugins, and that guarantee is
> already in the C plugin docs?

I think it would be better if we simply referred back to the C
documentation to avoid duplication.  Also an advantage of using
bitmasks.  That does require a rather larger change to the
documentation though.

> >  static int
> >-py_pwrite (void *handle, const void *buf,
> >-           uint32_t count, uint64_t offset)
> >+py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
> >+           uint32_t flags)
> >  {
> >    PyObject *obj = handle;
> >    PyObject *fn;
> >@@ -515,9 +546,19 @@ py_pwrite (void *handle, const void *buf,
> >    if (callback_defined ("pwrite", &fn)) {
> >      PyErr_Clear ();
> >-    r = PyObject_CallFunction (fn, "ONL", obj,
> >-                               PyByteArray_FromStringAndSize (buf, count),
> >-                               offset, NULL);
> >+    switch (py_api_version) {
> >+    case 1:
> >+      r = PyObject_CallFunction (fn, "ONL", obj,
> >+                                 PyByteArray_FromStringAndSize (buf, count),
> >+                                 offset, NULL);
> 
> Here, we could assert (flags == 0) (the FUA flag should not be set
> if the plugin uses v1 API).

Is that true?  The plugin asserts that it wants the v1 API, but we are
still using the v2 C API, whatever the plugin asks for?

I'm cautious about adding asserts unless they can never happen now or
in the future, because we don't really want an assert() happening on a
customer site running virt-v2v with the rhv-upload Python plugin.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/