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

[Eric Blake]

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