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/ _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
