3:14 p.m.
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/