3:30 p.m.
On 11/22/19 3:14 PM, Richard W.M. Jones wrote:
>> @@ -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.
Fair enough. I'm just making sure that our Python interface is at least
idiomatic to a Python programmer, rather than blatantly being a naive
translation of a C programmer.
>> - 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.
Pointing to the C docs, and focusing on just the bindings-induced
differences, is fine.
>> 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?
Hmm. If the user implements a 'can_fua' callback that returns 1 even
though they forgot to declare 'api_version', then the flag can indeed be
set. Perhaps that means our 'can_fua' C wrapper should have a version
1/2 difference in behavior (in v1, raise an error reminding the user to
fix their plugin to declare 'api_version', in v2 pass back the result),
so that we could indeed then use the assert with a guarantee that it
will not trigger on an arbitrary plugin.
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.
Understood.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org