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/