9:08 p.m.
Still more patches to come, but this fixes the heap leak I pointed out
this morning, and rebases my previous patches on top of that cleanup.
Eric Blake (8):
python: Improve doc comments for nbd.py
python: Plug uninit leak in nbd.Buffer.to_bytearray
python: Enhance tests of nbd.Buffer
python: Reformat generated methods.c in a few places
python: Make py_aio_buffer a private struct
python: Don't unwrap nbd.Buffer in nbd.py
python: Simplify python generator
python: Make nbd.Buffer lighter-weight
generator/Python.ml | 336 +++++++++++++++++-------------------
python/handle.c | 214 ++++++-----------------
python/t/500-aio-pread.py | 83 ++++++++-
python/t/510-aio-pwrite.py | 19 +-
python/t/580-aio-is-zero.py | 7 +
python/utils.c | 20 ++-
6 files changed, 336 insertions(+), 343 deletions(-)
--
2.36.1
9:08 p.m.
New subject: [libnbd PATCH v2 1/8] python: Improve doc comments for nbd.py
PEP257 recommends that doc comments start with a one line summary and
be a complete sentence. Tweak our existing comments to comply. It
also recommends the use of """ strings, and for u""" when a
string
contains Unicode; however, for since it is easier to write ' than " in
OCaml pr statements, we stick with ''' and u''' strings rather
than
follow the PEP completely.
Also, enhance the documentation for nbd.Buffer.from_bytearray to match
the semantic changes from commit d477f7c7, before we change those
semantics yet again in an upcoming patch (copying is inefficient, we
are moving towards reusing the underlying buffer directly).
---
generator/Python.ml | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 3f672ba..392be87 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -763,28 +763,33 @@ let
'''Asynchronous I/O persistent buffer'''
def __init__(self, len):
- '''allocate an uninitialized AIO buffer used for
nbd.aio_pread'''
+ '''Allocate an uninitialized AIO buffer used for
nbd.aio_pread.'''
self._o = libnbdmod.alloc_aio_buffer(len)
@classmethod
def from_bytearray(cls, ba):
- '''create an AIO buffer from a bytearray'''
+ '''Create an AIO buffer from a bytearray or other buffer-like
object.
+
+ If ba is not a buffer, it is tried as the parameter to the
+ bytearray constructor. Otherwise, ba is copied. Either way, the
+ resulting AIO buffer is independent from the original.
+ '''
o = libnbdmod.aio_buffer_from_bytearray(ba)
self = cls(0)
self._o = o
return self
def to_bytearray(self):
- '''copy an AIO buffer into a bytearray'''
+ '''Copy an AIO buffer into a bytearray.'''
return libnbdmod.aio_buffer_to_bytearray(self._o)
def size(self):
- '''return the size of an AIO buffer'''
+ '''Return the size of an AIO buffer.'''
return libnbdmod.aio_buffer_size(self._o)
def is_zero(self, offset=0, size=-1):
- '''
- Returns true if and only if all bytes in the buffer are zeroes.
+ '''Returns true if and only if all bytes in the buffer are zeroes.
+
Note that a freshly allocated buffer is uninitialized, not zero.
By default this tests the whole buffer, but you can restrict
@@ -801,11 +806,11 @@ let
'''NBD handle'''
def __init__(self):
- '''create a new NBD handle'''
+ '''Create a new NBD handle.'''
self._o = libnbdmod.create()
def __del__(self):
- '''close the NBD handle and underlying connection'''
+ '''Close the NBD handle and underlying connection.'''
if hasattr(self, '_o'):
libnbdmod.close(self._o)
@@ -855,7 +860,7 @@ let
let longdesc = Str.global_replace py_fn_rex "C<nbd.\\1>" longdesc
in
let longdesc = Str.global_replace py_const_rex "C<" longdesc in
let longdesc = pod2text longdesc in
- pr " '''â–¶ %s\n\n%s'''\n" shortdesc
(String.concat "\n" longdesc);
+ pr " u'''â–¶ %s\n\n%s'''\n" shortdesc
(String.concat "\n" longdesc);
pr " return libnbdmod.%s(" name;
pr_wrap ',' (fun () ->
pr "self._o";
--
2.36.1
8:10 a.m.
New subject: [libnbd PATCH v2 1/8] python: Improve doc comments for nbd.py
On Mon, Jun 06, 2022 at 09:08:26PM -0500, Eric Blake wrote:
PEP257 recommends that doc comments start with a one line summary
and
be a complete sentence. Tweak our existing comments to comply. It
also recommends the use of """ strings, and for u""" when a
string
contains Unicode; however, for since it is easier to write ' than " in
OCaml pr statements, we stick with ''' and u''' strings rather
than
follow the PEP completely.
Also, enhance the documentation for nbd.Buffer.from_bytearray to match
the semantic changes from commit d477f7c7, before we change those
semantics yet again in an upcoming patch (copying is inefficient, we
are moving towards reusing the underlying buffer directly).
---
generator/Python.ml | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 3f672ba..392be87 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -763,28 +763,33 @@ let
'''Asynchronous I/O persistent buffer'''
def __init__(self, len):
- '''allocate an uninitialized AIO buffer used for
nbd.aio_pread'''
+ '''Allocate an uninitialized AIO buffer used for
nbd.aio_pread.'''
self._o = libnbdmod.alloc_aio_buffer(len)
@classmethod
def from_bytearray(cls, ba):
- '''create an AIO buffer from a bytearray'''
+ '''Create an AIO buffer from a bytearray or other buffer-like
object.
+
+ If ba is not a buffer, it is tried as the parameter to the
+ bytearray constructor. Otherwise, ba is copied. Either way, the
+ resulting AIO buffer is independent from the original.
+ '''
o = libnbdmod.aio_buffer_from_bytearray(ba)
self = cls(0)
self._o = o
return self
def to_bytearray(self):
- '''copy an AIO buffer into a bytearray'''
+ '''Copy an AIO buffer into a bytearray.'''
return libnbdmod.aio_buffer_to_bytearray(self._o)
def size(self):
- '''return the size of an AIO buffer'''
+ '''Return the size of an AIO buffer.'''
return libnbdmod.aio_buffer_size(self._o)
def is_zero(self, offset=0, size=-1):
- '''
- Returns true if and only if all bytes in the buffer are zeroes.
+ '''Returns true if and only if all bytes in the buffer are zeroes.
+
Note that a freshly allocated buffer is uninitialized, not zero.
By default this tests the whole buffer, but you can restrict
@@ -801,11 +806,11 @@ let
'''NBD handle'''
def __init__(self):
- '''create a new NBD handle'''
+ '''Create a new NBD handle.'''
self._o = libnbdmod.create()
def __del__(self):
- '''close the NBD handle and underlying connection'''
+ '''Close the NBD handle and underlying connection.'''
if hasattr(self, '_o'):
libnbdmod.close(self._o)
@@ -855,7 +860,7 @@ let
let longdesc = Str.global_replace py_fn_rex "C<nbd.\\1>" longdesc
in
let longdesc = Str.global_replace py_const_rex "C<" longdesc in
let longdesc = pod2text longdesc in
- pr " '''â–¶ %s\n\n%s'''\n" shortdesc
(String.concat "\n" longdesc);
+ pr " u'''â–¶ %s\n\n%s'''\n" shortdesc
(String.concat "\n" longdesc);
pr " return libnbdmod.%s(" name;
pr_wrap ',' (fun () ->
pr "self._o";
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
9:08 p.m.
New subject: [libnbd PATCH v2 2/8] python: Plug uninit leak in nbd.Buffer.to_bytearray
When we created nbd.Buffer to handle both aio_pread and aio_pwrite, we
provided two different approaches to construct an instance. The
constructor nbd.Buffer(int) allocates uninitialized data, to be filled
in by aio_pread (rather than wasting time pre-zeroing the data just to
be changed again - although that speedup is only possible when opting
in to h.set_pread_initialized(false)). Meanwhile, the classmethod
nbd.Buffer.from_bytearray(...) allocates initialized data, to be
consumed by aio_pwrite. However, this scheme makes it trivially easy
to use the wrong instance creation approach for a given handle
operation, and thereby leak uninitialized heap contents into the
destination with something as simple-looking as
h.aio_pwrite(nbd.Buffer(512), 0). It doesn't help that while
bytearray(10) is all zeroes (a conscientious choice of the Python
language), and therefore nbd.Buffer.from_bytearray(10).is_zero() is
deterministically True, nbd.Buffer(10).is_zero() is non-deterministic;
nor that nbd.Buffer(10).to_bytearray() can allow Python code to play
with uninitialized data.
It is always a dangerous practice to leak heap contents; however, as
this leak is triggered only by poor client Python code, and not under
control of anything the server sends, we don't think it rises to the
level of the heap leak that was assigned CVE-2022-0485 as plugged back
in commit 8d444b41.
The solution employed here is to mark when a buffer has been
initialized, in nbd.Buffer.from_bytearray() and
h.aio_pread[_structured], as well as force-initialize an uninitialized
buffer before b.to_bytearray() or h.aio_pwrite. Furthermore, we can
make b.is_zero() pretend an uninitialized buffer is all zeroes (since
the user can no longer easily get at any other contents).
The approach is not bulletproof - if h.aio_pread triggers a subsequent
read error in the C code (such as client-side bounds check or server
error response) AND the user has disabled h.set_pread_initialized(),
we've already marked the buffer initialized at that point even though
the buffer may not be fully written. But that's okay - we've already
documented that a user opting in to that setting is responsible for
safe buffer practices, and should not dereference a read buffer when
an error is reported while retiring the command.
Generated code changes as follows:
| --- python/methods.h.bak 2022-06-06 07:43:33.167181594 -0500
| +++ python/methods.h 2022-06-06 07:43:40.494190038 -0500
| @@ -31,6 +31,7 @@
| struct py_aio_buffer {
| Py_ssize_t len;
| void *data;
| + bool initialized;
| };
|
| extern char **nbd_internal_py_get_string_list (PyObject *);
| --- python/methods.c.bak 2022-06-06 07:43:33.165181592 -0500
| +++ python/methods.c 2022-06-06 07:56:25.109094074 -0500
| @@ -3201,6 +3201,7 @@ nbd_internal_py_aio_pread (PyObject *sel
| completion_user_data->buf = buf;
| offset_u64 = offset;
|
| + buf_buf->initialized = true;
| ret = nbd_aio_pread (h, buf_buf->data, buf_buf->len, offset_u64, completion,
flags_u32);
| completion_user_data = NULL;
| if (ret == -1) {
| @@ -3274,6 +3275,7 @@ nbd_internal_py_aio_pread_structured (Py
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
|
| + buf_buf->initialized = true;
| ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len, offset_u64,
chunk, completion, flags_u32);
| chunk_user_data = NULL;
| completion_user_data = NULL;
| @@ -3335,6 +3337,10 @@ nbd_internal_py_aio_pwrite (PyObject *se
| completion_user_data->buf = buf;
| offset_u64 = offset;
|
| + if (!buf_buf->initialized) {
| + memset (buf_buf->data, 0, buf_buf->len);
| + buf_buf->initialized = true;
| + }
| ret = nbd_aio_pwrite (h, buf_buf->data, buf_buf->len, offset_u64, completion,
flags_u32);
| completion_user_data = NULL;
| if (ret == -1) {
| --- python/nbd.py.bak 2022-06-06 07:43:33.169181596 -0500
| +++ python/nbd.py 2022-06-06 07:43:40.516190063 -0500
| @@ -150,7 +150,9 @@ class Buffer(object):
| def is_zero(self, offset=0, size=-1):
| '''Returns true if and only if all bytes in the buffer are zeroes.
|
| - Note that a freshly allocated buffer is uninitialized, not zero.
| + Note that although a freshly allocated buffer is uninitialized,
| + this will report it as all zeroes, as it will be force-initialized
| + to zero before any code that can access the buffer's contents.
|
| By default this tests the whole buffer, but you can restrict
| the test to a sub-range of the buffer using the optional
Fixes: f05b8ec5 ("python: Change aio_buffer into nbd.Buffer class.", v0.9.8)
---
generator/Python.ml | 23 +++++++++++++++++++----
python/handle.c | 9 ++++++++-
python/t/510-aio-pwrite.py | 13 ++++++++++++-
python/t/580-aio-is-zero.py | 7 +++++++
4 files changed, 46 insertions(+), 6 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 392be87..8239803 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -37,6 +37,7 @@ let
struct py_aio_buffer {
Py_ssize_t len;
void *data;
+ bool initialized;
};
extern char **nbd_internal_py_get_string_list (PyObject *);
@@ -79,7 +80,7 @@ let
"aio_buffer_from_bytearray";
"aio_buffer_to_bytearray";
"aio_buffer_size";
- "aio_buffer_is_zero" ] @ List.map fst handle_calls);
+ "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n";
pr "#endif /* LIBNBD_METHODS_H */\n"
@@ -111,7 +112,7 @@ let
"aio_buffer_from_bytearray";
"aio_buffer_to_bytearray";
"aio_buffer_size";
- "aio_buffer_is_zero" ] @ List.map fst handle_calls);
+ "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr " { NULL, NULL, 0, NULL }\n";
pr "};\n";
pr "\n";
@@ -408,6 +409,7 @@ let
pr "))\n";
pr " goto out;\n";
+ (* Two passes over parameters. Any 'goto err' must be in first pass. *)
pr " h = get_handle (py_h);\n";
pr " if (!h) goto out;\n";
List.iter (
@@ -475,7 +477,18 @@ let
) args;
pr "\n";
- (* Call the underlying C function. *)
+ (* Second pass, and call the underlying C function. *)
+ List.iter (
+ function
+ | BytesPersistIn (n, _) ->
+ pr " if (!%s_buf->initialized) {\n" n;
+ pr " memset (%s_buf->data, 0, %s_buf->len);\n" n n;
+ pr " %s_buf->initialized = true;\n" n;
+ pr " }\n"
+ | BytesPersistOut (n, _) ->
+ pr " %s_buf->initialized = true;\n" n
+ | _ -> ()
+ ) args;
pr " ret = nbd_%s (h" name;
List.iter (
function
@@ -790,7 +803,9 @@ let
def is_zero(self, offset=0, size=-1):
'''Returns true if and only if all bytes in the buffer are zeroes.
- Note that a freshly allocated buffer is uninitialized, not zero.
+ Note that although a freshly allocated buffer is uninitialized,
+ this will report it as all zeroes, as it will be force-initialized
+ to zero before any code that can access the buffer's contents.
By default this tests the whole buffer, but you can restrict
the test to a sub-range of the buffer using the optional
diff --git a/python/handle.c b/python/handle.c
index ed04b76..d42a563 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -127,6 +127,7 @@ nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args)
return NULL;
}
+ buf->initialized = false;
if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer",
&buf->len)) {
free (buf);
@@ -202,6 +203,7 @@ nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject
*args)
}
memcpy (buf->data, data, len);
Py_XDECREF (arr);
+ buf->initialized = true;
ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer);
if (ret == NULL) {
@@ -228,6 +230,11 @@ nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject
*args)
if (buf == NULL)
return NULL;
+ if (!buf->initialized) {
+ memset (buf->data, 0, buf->len);
+ buf->initialized = true;
+ }
+
return PyByteArray_FromStringAndSize (buf->data, buf->len);
}
@@ -288,7 +295,7 @@ nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args)
return NULL;
}
- if (is_zero (buf->data + offset, size))
+ if (!buf->initialized || is_zero (buf->data + offset, size))
Py_RETURN_TRUE;
else
Py_RETURN_FALSE;
diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
index 438cc54..89599fc 100644
--- a/python/t/510-aio-pwrite.py
+++ b/python/t/510-aio-pwrite.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -50,4 +50,15 @@ with open(datafile, "rb") as f:
assert buf == content
+# Also check that an uninitialized buffer doesn't leak heap contents
+buf3 = nbd.Buffer(512)
+cookie = h.aio_pwrite(buf3, 0, flags=nbd.CMD_FLAG_FUA)
+while not h.aio_command_completed(cookie):
+ h.poll(-1)
+
+with open(datafile, "rb") as f:
+ content = f.read()
+
+assert nbd.Buffer.from_bytearray(content).is_zero()
+
os.unlink(datafile)
diff --git a/python/t/580-aio-is-zero.py b/python/t/580-aio-is-zero.py
index 49dea5e..1a9a7d3 100644
--- a/python/t/580-aio-is-zero.py
+++ b/python/t/580-aio-is-zero.py
@@ -69,3 +69,10 @@ assert not buf.is_zero(2**20-1, 1)
assert buf.is_zero(2**20, 1)
assert not buf.is_zero(0, 1)
assert buf.is_zero(2**21-1, 1)
+
+# A buffer created with only a size is generally uninitialized until
+# used with aio_pread; but it will be zeroed if accessed prematurely
+buf = nbd.Buffer(1024)
+assert buf.size() == 1024
+assert buf.is_zero()
+assert nbd.Buffer.from_bytearray(buf.to_bytearray()).is_zero()
--
2.36.1
8:13 a.m.
New subject: [libnbd PATCH v2 2/8] python: Plug uninit leak in nbd.Buffer.to_bytearray
On Mon, Jun 06, 2022 at 09:08:27PM -0500, Eric Blake wrote:
@@ -79,7 +80,7 @@ let
"aio_buffer_from_bytearray";
"aio_buffer_to_bytearray";
"aio_buffer_size";
- "aio_buffer_is_zero" ] @ List.map fst handle_calls);
+ "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n";
pr "#endif /* LIBNBD_METHODS_H */\n"
@@ -111,7 +112,7 @@ let
"aio_buffer_from_bytearray";
"aio_buffer_to_bytearray";
"aio_buffer_size";
- "aio_buffer_is_zero" ] @ List.map fst handle_calls);
+ "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr " { NULL, NULL, 0, NULL }\n";
pr "};\n";
pr "\n";
Unintended whitespace changes? Without those two hunks:
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
8:32 a.m.
New subject: [libnbd PATCH v2 2/8] python: Plug uninit leak in nbd.Buffer.to_bytearray
On Tue, Jun 07, 2022 at 02:13:23PM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:27PM -0500, Eric Blake wrote:
> @@ -79,7 +80,7 @@ let
> "aio_buffer_from_bytearray";
> "aio_buffer_to_bytearray";
> "aio_buffer_size";
> - "aio_buffer_is_zero" ] @ List.map fst handle_calls);
> + "aio_buffer_is_zero"] @ List.map fst handle_calls);
>
> pr "\n";
> pr "#endif /* LIBNBD_METHODS_H */\n"
> @@ -111,7 +112,7 @@ let
> "aio_buffer_from_bytearray";
> "aio_buffer_to_bytearray";
> "aio_buffer_size";
> - "aio_buffer_is_zero" ] @ List.map fst handle_calls);
> + "aio_buffer_is_zero"] @ List.map fst handle_calls);
> pr " { NULL, NULL, 0, NULL }\n";
> pr "};\n";
> pr "\n";
Unintended whitespace changes? Without those two hunks:
Yeah, I was playing with adding aio_buffer_is_init to let the user
probe the status of an nbd.Buffer, then changed my mind, and it shows.
Will fix.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
9:06 a.m.
New subject: [libnbd PATCH v2 2/8] python: Plug uninit leak in nbd.Buffer.to_bytearray
On Mon, Jun 06, 2022 at 09:08:27PM -0500, Eric Blake wrote:
The solution employed here is to mark when a buffer has been
initialized, in nbd.Buffer.from_bytearray() and
h.aio_pread[_structured], as well as force-initialize an uninitialized
buffer before b.to_bytearray() or h.aio_pwrite. Furthermore, we can
make b.is_zero() pretend an uninitialized buffer is all zeroes (since
the user can no longer easily get at any other contents).
@@ -288,7 +295,7 @@ nbd_internal_py_aio_buffer_is_zero (PyObject
*self, PyObject *args)
return NULL;
}
- if (is_zero (buf->data + offset, size))
+ if (!buf->initialized || is_zero (buf->data + offset, size))
Py_RETURN_TRUE;
else
Py_RETURN_FALSE;
I just noticed this function has an odd order of execution - if size
is 0, it returns true even if offset is out-of-bounds. It would also
be more Pythonic if we changed offset/size to instead be more like
Python range start/stop parameters (particularly since future patches
can then compute memoryview(buffer)[start:end] as the bytes to check
for being zero), but that's an API change. I'll have to think about
that one more...
But that doesn't change the content of this patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
9:08 p.m.
New subject: [libnbd PATCH v2 3/8] python: Enhance tests of nbd.Buffer
Add some more coverage of existing behavior, so we can better track
that we are not introducing unintended changes in upcoming patches.
---
python/t/500-aio-pread.py | 83 +++++++++++++++++++++++++++++++++++---
python/t/510-aio-pwrite.py | 6 +++
2 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
index fc1abad..c236163 100644
--- a/python/t/500-aio-pread.py
+++ b/python/t/500-aio-pread.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -19,17 +19,25 @@ import nbd
h = nbd.NBD()
h.connect_command(["nbdkit", "-s", "--exit-with-parent",
"-v",
- "pattern", "size=512"])
+ "pattern", "size=1024"])
buf = nbd.Buffer(512)
cookie = h.aio_pread(buf, 0)
while not h.aio_command_completed(cookie):
h.poll(-1)
-buf = buf.to_bytearray()
+buf1 = buf.to_bytearray()
-print("%r" % buf)
+# Prove that .to_bytearray() defaults to copying, even if buf is reused
-assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
+cookie = h.aio_pread(buf, 512)
+while not h.aio_command_completed(cookie):
+ h.poll(-1)
+
+buf2 = buf.to_bytearray()
+
+print("%r" % buf1)
+
+assert buf1 == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
+ b'\x00\x00\x00\x00\x00\x00\x00\x08'
+ b'\x00\x00\x00\x00\x00\x00\x00\x10'
+ b'\x00\x00\x00\x00\x00\x00\x00\x18'
@@ -93,3 +101,68 @@ assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
+ b'\x00\x00\x00\x00\x00\x00\x01\xe8'
+ b'\x00\x00\x00\x00\x00\x00\x01\xf0'
+ b'\x00\x00\x00\x00\x00\x00\x01\xf8')
+
+assert buf2 == (b'\x00\x00\x00\x00\x00\x00\x02\x00'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x08'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x10'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x18'
+ + b'\x00\x00\x00\x00\x00\x00\x02 '
+ + b'\x00\x00\x00\x00\x00\x00\x02('
+ + b'\x00\x00\x00\x00\x00\x00\x020'
+ + b'\x00\x00\x00\x00\x00\x00\x028'
+ + b'\x00\x00\x00\x00\x00\x00\x02@'
+ + b'\x00\x00\x00\x00\x00\x00\x02H'
+ + b'\x00\x00\x00\x00\x00\x00\x02P'
+ + b'\x00\x00\x00\x00\x00\x00\x02X'
+ + b'\x00\x00\x00\x00\x00\x00\x02`'
+ + b'\x00\x00\x00\x00\x00\x00\x02h'
+ + b'\x00\x00\x00\x00\x00\x00\x02p'
+ + b'\x00\x00\x00\x00\x00\x00\x02x'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x80'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x88'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x90'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x98'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xa0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xa8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xb0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xb8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xc0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xc8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xd0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xd8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xe0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xe8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xf0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xf8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x00'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x08'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x10'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x18'
+ + b'\x00\x00\x00\x00\x00\x00\x03 '
+ + b'\x00\x00\x00\x00\x00\x00\x03('
+ + b'\x00\x00\x00\x00\x00\x00\x030'
+ + b'\x00\x00\x00\x00\x00\x00\x038'
+ + b'\x00\x00\x00\x00\x00\x00\x03@'
+ + b'\x00\x00\x00\x00\x00\x00\x03H'
+ + b'\x00\x00\x00\x00\x00\x00\x03P'
+ + b'\x00\x00\x00\x00\x00\x00\x03X'
+ + b'\x00\x00\x00\x00\x00\x00\x03`'
+ + b'\x00\x00\x00\x00\x00\x00\x03h'
+ + b'\x00\x00\x00\x00\x00\x00\x03p'
+ + b'\x00\x00\x00\x00\x00\x00\x03x'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x80'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x88'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x90'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x98'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xa0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xa8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xb0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xb8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xc0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xc8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xd0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xd8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xe0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xe8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xf0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xf8')
diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
index 89599fc..d09e249 100644
--- a/python/t/510-aio-pwrite.py
+++ b/python/t/510-aio-pwrite.py
@@ -45,6 +45,12 @@ while not h.aio_command_completed(cookie):
assert buf == buf2.to_bytearray()
+# Check that .from_bytearray() defaults to copying
+buf[511] = 0x55
+assert buf != buf1.to_bytearray()
+buf[511] = 0xAA
+assert buf == buf1.to_bytearray()
+
with open(datafile, "rb") as f:
content = f.read()
--
2.36.1
8:14 a.m.
New subject: [libnbd PATCH v2 3/8] python: Enhance tests of nbd.Buffer
On Mon, Jun 06, 2022 at 09:08:28PM -0500, Eric Blake wrote:
Add some more coverage of existing behavior, so we can better track
that we are not introducing unintended changes in upcoming patches.
---
python/t/500-aio-pread.py | 83 +++++++++++++++++++++++++++++++++++---
python/t/510-aio-pwrite.py | 6 +++
2 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
index fc1abad..c236163 100644
--- a/python/t/500-aio-pread.py
+++ b/python/t/500-aio-pread.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -19,17 +19,25 @@ import nbd
h = nbd.NBD()
h.connect_command(["nbdkit", "-s", "--exit-with-parent",
"-v",
- "pattern", "size=512"])
+ "pattern", "size=1024"])
buf = nbd.Buffer(512)
cookie = h.aio_pread(buf, 0)
while not h.aio_command_completed(cookie):
h.poll(-1)
-buf = buf.to_bytearray()
+buf1 = buf.to_bytearray()
-print("%r" % buf)
+# Prove that .to_bytearray() defaults to copying, even if buf is reused
-assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
+cookie = h.aio_pread(buf, 512)
+while not h.aio_command_completed(cookie):
+ h.poll(-1)
+
+buf2 = buf.to_bytearray()
+
+print("%r" % buf1)
+
+assert buf1 == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
+ b'\x00\x00\x00\x00\x00\x00\x00\x08'
+ b'\x00\x00\x00\x00\x00\x00\x00\x10'
+ b'\x00\x00\x00\x00\x00\x00\x00\x18'
@@ -93,3 +101,68 @@ assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
+ b'\x00\x00\x00\x00\x00\x00\x01\xe8'
+ b'\x00\x00\x00\x00\x00\x00\x01\xf0'
+ b'\x00\x00\x00\x00\x00\x00\x01\xf8')
+
+assert buf2 == (b'\x00\x00\x00\x00\x00\x00\x02\x00'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x08'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x10'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x18'
+ + b'\x00\x00\x00\x00\x00\x00\x02 '
+ + b'\x00\x00\x00\x00\x00\x00\x02('
+ + b'\x00\x00\x00\x00\x00\x00\x020'
+ + b'\x00\x00\x00\x00\x00\x00\x028'
+ + b'\x00\x00\x00\x00\x00\x00\x02@'
+ + b'\x00\x00\x00\x00\x00\x00\x02H'
+ + b'\x00\x00\x00\x00\x00\x00\x02P'
+ + b'\x00\x00\x00\x00\x00\x00\x02X'
+ + b'\x00\x00\x00\x00\x00\x00\x02`'
+ + b'\x00\x00\x00\x00\x00\x00\x02h'
+ + b'\x00\x00\x00\x00\x00\x00\x02p'
+ + b'\x00\x00\x00\x00\x00\x00\x02x'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x80'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x88'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x90'
+ + b'\x00\x00\x00\x00\x00\x00\x02\x98'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xa0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xa8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xb0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xb8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xc0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xc8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xd0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xd8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xe0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xe8'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xf0'
+ + b'\x00\x00\x00\x00\x00\x00\x02\xf8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x00'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x08'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x10'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x18'
+ + b'\x00\x00\x00\x00\x00\x00\x03 '
+ + b'\x00\x00\x00\x00\x00\x00\x03('
+ + b'\x00\x00\x00\x00\x00\x00\x030'
+ + b'\x00\x00\x00\x00\x00\x00\x038'
+ + b'\x00\x00\x00\x00\x00\x00\x03@'
+ + b'\x00\x00\x00\x00\x00\x00\x03H'
+ + b'\x00\x00\x00\x00\x00\x00\x03P'
+ + b'\x00\x00\x00\x00\x00\x00\x03X'
+ + b'\x00\x00\x00\x00\x00\x00\x03`'
+ + b'\x00\x00\x00\x00\x00\x00\x03h'
+ + b'\x00\x00\x00\x00\x00\x00\x03p'
+ + b'\x00\x00\x00\x00\x00\x00\x03x'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x80'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x88'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x90'
+ + b'\x00\x00\x00\x00\x00\x00\x03\x98'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xa0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xa8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xb0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xb8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xc0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xc8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xd0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xd8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xe0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xe8'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xf0'
+ + b'\x00\x00\x00\x00\x00\x00\x03\xf8')
diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
index 89599fc..d09e249 100644
--- a/python/t/510-aio-pwrite.py
+++ b/python/t/510-aio-pwrite.py
@@ -45,6 +45,12 @@ while not h.aio_command_completed(cookie):
assert buf == buf2.to_bytearray()
+# Check that .from_bytearray() defaults to copying
+buf[511] = 0x55
+assert buf != buf1.to_bytearray()
+buf[511] = 0xAA
+assert buf == buf1.to_bytearray()
+
with open(datafile, "rb") as f:
content = f.read()
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
8:48 a.m.
New subject: [libnbd PATCH v2 3/8] python: Enhance tests of nbd.Buffer
On Tue, Jun 07, 2022 at 02:14:36PM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:28PM -0500, Eric Blake wrote:
> Add some more coverage of existing behavior, so we can better track
> that we are not introducing unintended changes in upcoming patches.
> ---
> python/t/500-aio-pread.py | 83 +++++++++++++++++++++++++++++++++++---
> python/t/510-aio-pwrite.py | 6 +++
> 2 files changed, 84 insertions(+), 5 deletions(-)
>
> diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
> index fc1abad..c236163 100644
> --- a/python/t/500-aio-pread.py
> +print("%r" % buf1)
> +
> +assert buf1 == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
> + b'\x00\x00\x00\x00\x00\x00\x00\x08'
> + b'\x00\x00\x00\x00\x00\x00\x00\x10'
> + b'\x00\x00\x00\x00\x00\x00\x00\x18'
> @@ -93,3 +101,68 @@ assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00'
> + b'\x00\x00\x00\x00\x00\x00\x01\xe8'
> + b'\x00\x00\x00\x00\x00\x00\x01\xf0'
> + b'\x00\x00\x00\x00\x00\x00\x01\xf8')
For this one, I may try to shorten the test by using:
import array from array
a = array('q', range(0, 512, 8))
if sys.byteorder == 'little':
a.byteswap()
assert buf1 == memoryview(a).cast('B')
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
9:08 p.m.
New subject: [libnbd PATCH v2 4/8] python: Reformat generated methods.c in a few places
To make argument parsing/building easier to follow, and make it so
that future type changes touch fewer lines of code, consolidate
several passes of List.iter to instead produce a list of tuples in one
pass. Now the python conversion character(s) are next to the code
that character matches. Compress the strings passed to
PyArg_ParseTuple and Py_BuildValue. Utilize py_wrap to avoid
generated lines that exceed 80 columns.
The change is solely in the formatting; no behavioral changes are
intended. The generated diff includes chunks such as:
| @@ -84,7 +84,8 @@ chunk_wrapper (void *user_data, const vo
| Py_DECREF (py_error_mod);
| if (!py_error) { PyErr_PrintEx (0); goto out; }
|
| - py_args = Py_BuildValue ("(" "y#" "K" "I"
"O" ")", subbuf, (int) count, offset, status, py_error);
| + py_args = Py_BuildValue ("(y#KIO)", subbuf, (int) count, offset, status,
| + py_error);
| if (!py_args) { PyErr_PrintEx (0); goto out; }
|
| py_save = PyGILState_Ensure ();
...
| @@ -1054,7 +1031,8 @@ nbd_internal_py_set_tls_psk_file (PyObje
| }
|
| PyObject *
| -nbd_internal_py_set_request_structured_replies (PyObject *self, PyObject *args)
| +nbd_internal_py_set_request_structured_replies (PyObject *self,
| + PyObject *args)
| {
| PyObject *py_h;
| struct nbd_handle *h;
...
| @@ -2344,8 +2281,7 @@ nbd_internal_py_pread_structured (PyObje
| uint32_t flags_u32;
| unsigned int flags; /* really uint32_t */
|
| - if (!PyArg_ParseTuple (args, "O" "n" "K" "O"
"I"
| - ":nbd_pread_structured",
| + if (!PyArg_ParseTuple (args, "OnKOI:nbd_pread_structured",
| &py_h, &count, &offset, &py_chunk_fn,
&flags))
| goto out;
| h = get_handle (py_h);
| @@ -2365,7 +2301,8 @@ nbd_internal_py_pread_structured (PyObje
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
|
| - ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count, offset_u64, chunk,
flags_u32);
| + ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count,
| + offset_u64, chunk, flags_u32);
| chunk_user_data = NULL;
| if (ret == -1) {
| raise_exception ();
Suggested-by: Richard W.M. Jones <rjones(a)redhat.com>
---
generator/Python.ml | 201 ++++++++++++++++++--------------------------
1 file changed, 81 insertions(+), 120 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 8239803..6244c8e 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -209,30 +209,31 @@ let
) cbargs;
pr "\n";
- pr " py_args = Py_BuildValue (\"(\"";
- List.iter (
- function
- | CBArrayAndLen (UInt32 n, len) -> pr " \"O\""
- | CBBytesIn (n, len) -> pr " \"y#\""
- | CBInt n -> pr " \"i\""
- | CBInt64 n -> pr " \"L\""
- | CBMutable (Int n) -> pr " \"O\""
- | CBString n -> pr " \"s\""
- | CBUInt n -> pr " \"I\""
- | CBUInt64 n -> pr " \"K\""
- | CBArrayAndLen _ | CBMutable _ -> assert false
- ) cbargs;
- pr " \")\"";
- List.iter (
- function
- | CBArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
- | CBBytesIn (n, len) -> pr ", %s, (int) %s" n len
- | CBMutable (Int n) -> pr ", py_%s" n
- | CBInt n | CBInt64 n
- | CBString n
- | CBUInt n | CBUInt64 n -> pr ", %s" n
- | CBArrayAndLen _ | CBMutable _ -> assert false
- ) cbargs;
+ let params =
+ List.map (
+ function
+ | CBArrayAndLen (UInt32 n, _) -> "O", sprintf "py_%s" n
+ | CBBytesIn (n, len) -> "y#", sprintf "%s, (int) %s" n len
+ | CBInt n -> "i", n
+ | CBInt64 n -> "L", n
+ | CBMutable (Int n) -> "O", sprintf "py_%s" n
+ | CBString n -> "s", n
+ | CBUInt n -> "I", n
+ | CBUInt64 n -> "K", n
+ | CBArrayAndLen _ | CBMutable _ -> assert false
+ ) cbargs in
+ pr " py_args = Py_BuildValue (";
+ pr_wrap ',' (fun () ->
+ pr "\"(";
+ List.iter (
+ function
+ | n, _ -> pr "%s" n
+ ) params;
+ pr ")\"";
+ List.iter (
+ function
+ | _, n -> pr ", %s" n
+ ) params);
pr ");\n";
pr " if (!py_args) { PyErr_PrintEx (0); goto out; }\n";
pr "\n";
@@ -284,7 +285,10 @@ let
(* Generate the Python binding. *)
let print_python_binding name { args; optargs; ret; may_set_error } =
pr "PyObject *\n";
- pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name;
+ pr "nbd_internal_py_%s (" name;
+ pr_wrap ',' (fun () ->
+ pr "PyObject *self, PyObject *args");
+ pr ")\n";
pr "{\n";
pr " PyObject *py_h;\n";
pr " struct nbd_handle *h;\n";
@@ -353,59 +357,52 @@ let
pr "\n";
(* Parse the Python parameters. *)
- pr " if (!PyArg_ParseTuple (args, \"O\"";
+ let params =
+ List.map (
+ function
+ | Bool n -> "p", sprintf "&%s" n, n
+ | BytesIn (n, _) -> "y*", sprintf "&%s" n, sprintf
"%s.buf, %s.len" n n
+ | BytesOut (n, count) ->
+ "n", sprintf "&%s" count,
+ sprintf "PyByteArray_AS_STRING (%s), %s" n count
+ | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
+ "O", sprintf "&%s" n, sprintf "%s_buf->data,
%s_buf->len" n n
+ | Closure { cbname } -> "O", sprintf "&py_%s_fn" cbname,
cbname
+ | Enum (n, _) -> "i", sprintf "&%s" n, n
+ | Flags (n, _) -> "I", sprintf "&%s" n, sprintf
"%s_u32" n
+ | Fd n | Int n -> "i", sprintf "&%s" n, n
+ | Int64 n -> "L", sprintf "&%s" n, sprintf
"%s_i64" n
+ | Path n -> "O&", sprintf "PyUnicode_FSConverter,
&py_%s" n, n
+ | SizeT n -> "n", sprintf "&%s" n, sprintf
"(size_t)%s" n
+ | SockAddrAndLen (n, _) ->
+ "O", sprintf "&%s" n,
+ sprintf "(struct sockaddr *) &%s_sa, %s_len" n n
+ | String n -> "s", sprintf "&%s" n, n
+ | StringList n -> "O", sprintf "&py_%s" n, n
+ | UInt n | UIntPtr n -> "I", sprintf "&%s" n, n
+ | UInt32 n -> "I", sprintf "&%s" n, sprintf
"%s_u32" n
+ | UInt64 n -> "K", sprintf "&%s" n, sprintf
"%s_u64" n
+ ) args in
+ let optparams =
+ List.map (
+ function
+ | OClosure { cbname } -> "O", sprintf "&py_%s_fn"
cbname, cbname
+ | OFlags (n, _, _) -> "I", sprintf "&%s" n, sprintf
"%s_u32" n
+ ) optargs in
+ let params = params @ optparams in
+ pr " if (!PyArg_ParseTuple (args, \"O";
List.iter (
function
- | Bool n -> pr " \"p\""
- | BytesIn (n, _) -> pr " \"y*\""
- | BytesPersistIn (n, _) -> pr " \"O\""
- | BytesOut (_, count) -> pr " \"n\""
- | BytesPersistOut (_, count) -> pr " \"O\""
- | Closure _ -> pr " \"O\""
- | Enum _ -> pr " \"i\""
- | Flags _ -> pr " \"I\""
- | Fd n | Int n -> pr " \"i\""
- | Int64 n -> pr " \"L\""
- | Path n -> pr " \"O&\""
- | SizeT n -> pr " \"n\""
- | SockAddrAndLen (n, _) -> pr " \"O\""
- | String n -> pr " \"s\""
- | StringList n -> pr " \"O\""
- | UInt n | UIntPtr n -> pr " \"I\""
- | UInt32 n -> pr " \"I\""
- | UInt64 n -> pr " \"K\""
- ) args;
- List.iter (
- function
- | OClosure _ -> pr " \"O\""
- | OFlags _ -> pr " \"I\""
- ) optargs;
- pr "\n";
- pr " \":nbd_%s\",\n" name;
- pr " &py_h";
- List.iter (
- function
- | Bool n -> pr ", &%s" n
- | BytesIn (n, _) | BytesPersistIn (n, _)
- | BytesPersistOut (n, _) -> pr ", &%s" n
- | BytesOut (_, count) -> pr ", &%s" count
- | Closure { cbname } -> pr ", &py_%s_fn" cbname
- | Enum (n, _) -> pr ", &%s" n
- | Flags (n, _) -> pr ", &%s" n
- | Fd n | Int n | SizeT n | Int64 n -> pr ", &%s" n
- | Path n -> pr ", PyUnicode_FSConverter, &py_%s" n
- | SockAddrAndLen (n, _) -> pr ", &%s" n
- | String n -> pr ", &%s" n
- | StringList n -> pr ", &py_%s" n
- | UInt n | UIntPtr n -> pr ", &%s" n
- | UInt32 n -> pr ", &%s" n
- | UInt64 n -> pr ", &%s" n
- ) args;
- List.iter (
- function
- | OClosure { cbname } -> pr ", &py_%s_fn" cbname
- | OFlags (n, _, _) -> pr ", &%s" n
- ) optargs;
+ | n, _, _ -> pr "%s" n
+ ) params;
+ pr ":nbd_%s\",\n" name;
+ pr " ";
+ pr_wrap ',' (fun () ->
+ pr "&py_h";
+ List.iter (
+ function
+ | _, n, _ -> pr ", %s" n
+ ) params);
pr "))\n";
pr " goto out;\n";
@@ -489,33 +486,13 @@ let
pr " %s_buf->initialized = true;\n" n
| _ -> ()
) args;
- pr " ret = nbd_%s (h" name;
- List.iter (
- function
- | Bool n -> pr ", %s" n
- | BytesIn (n, _) -> pr ", %s.buf, %s.len" n n
- | BytesOut (n, count) -> pr ", PyByteArray_AS_STRING (%s), %s" n count
- | BytesPersistIn (n, _)
- | BytesPersistOut (n, _) -> pr ", %s_buf->data, %s_buf->len" n n
- | Closure { cbname } -> pr ", %s" cbname
- | Enum (n, _) -> pr ", %s" n
- | Flags (n, _) -> pr ", %s_u32" n
- | Fd n | Int n -> pr ", %s" n
- | Int64 n -> pr ", %s_i64" n
- | Path n -> pr ", %s" n
- | SizeT n -> pr ", (size_t)%s" n
- | SockAddrAndLen (n, _) -> pr ", (struct sockaddr *) &%s_sa, %s_len"
n n
- | String n -> pr ", %s" n
- | StringList n -> pr ", %s" n
- | UInt n | UIntPtr n -> pr ", %s" n
- | UInt32 n -> pr ", %s_u32" n
- | UInt64 n -> pr ", %s_u64" n
- ) args;
- List.iter (
- function
- | OClosure { cbname } -> pr ", %s" cbname
- | OFlags (n, _, _) -> pr ", %s_u32" n
- ) optargs;
+ pr " ret = nbd_%s (" name;
+ pr_wrap ',' (fun () ->
+ pr "h";
+ List.iter (
+ function
+ | _, _, n -> pr ", %s" n
+ ) params);
pr ");\n";
List.iter (
function
@@ -543,23 +520,7 @@ let
pr " py_ret = %s;\n" n;
pr " %s = NULL;\n" n;
use_ret := false
- | Bool _
- | BytesIn _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Enum _
- | Flags _
- | Fd _ | Int _
- | Int64 _
- | Path _
- | SockAddrAndLen _
- | SizeT _
- | String _
- | StringList _
- | UInt _
- | UIntPtr _
- | UInt32 _
- | UInt64 _ -> ()
+ | _ -> ()
) args;
if !use_ret then (
match ret with
--
2.36.1
3 a.m.
New subject: [libnbd PATCH v2 4/8] python: Reformat generated methods.c in a few places
On Mon, Jun 06, 2022 at 09:08:29PM -0500, Eric Blake wrote:
+ pr ":nbd_%s\",\n" name;
You could put this pr (but without the \n) ...
+ pr " ";
+ pr_wrap ',' (fun () ->
... inside pr_wrap here, and it would mean you wouldn't need to print
spaces to indent (because pr_wrap should do it for you).
It all looks sensible and equivalent to the old code, and the output
is cleaner too, so:
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
8:53 a.m.
New subject: [libnbd PATCH v2 4/8] python: Reformat generated methods.c in a few places
On Tue, Jun 07, 2022 at 09:00:08AM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:29PM -0500, Eric Blake wrote:
> + pr ":nbd_%s\",\n" name;
You could put this pr (but without the \n) ...
> + pr " ";
> + pr_wrap ',' (fun () ->
... inside pr_wrap here, and it would mean you wouldn't need to print
spaces to indent (because pr_wrap should do it for you).
Will do. It rearranges a few more lines of generated code (now the
&py_h argument can sometimes be a line earlier), but is still legible.
It all looks sensible and equivalent to the old code, and the output
is cleaner too, so:
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
I get when we have more than one statement to bundle how the OCaml
(fun () -> ... ) construct makes sense, but do we need that for this hunk?
@@ -284,7 +285,10 @@ let
(* Generate the Python binding. *)
let print_python_binding name { args; optargs; ret; may_set_error } =
pr "PyObject *\n";
- pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name;
+ pr "nbd_internal_py_%s (" name;
+ pr_wrap ',' (fun () ->
+ pr "PyObject *self, PyObject *args");
+ pr ")\n";
pr "{\n";
pr " PyObject *py_h;\n";
pr " struct nbd_handle *h;\n";
or is there a shorter way to write that one?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
9 a.m.
New subject: [libnbd PATCH v2 4/8] python: Reformat generated methods.c in a few places
On Tue, Jun 07, 2022 at 08:53:23AM -0500, Eric Blake wrote:
On Tue, Jun 07, 2022 at 09:00:08AM +0100, Richard W.M. Jones wrote:
> On Mon, Jun 06, 2022 at 09:08:29PM -0500, Eric Blake wrote:
> > + pr ":nbd_%s\",\n" name;
>
> You could put this pr (but without the \n) ...
>
> > + pr " ";
> > + pr_wrap ',' (fun () ->
>
> ... inside pr_wrap here, and it would mean you wouldn't need to print
> spaces to indent (because pr_wrap should do it for you).
Will do. It rearranges a few more lines of generated code (now the
&py_h argument can sometimes be a line earlier), but is still legible.
>
> It all looks sensible and equivalent to the old code, and the output
> is cleaner too, so:
>
> Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
>
I get when we have more than one statement to bundle how the OCaml
(fun () -> ... ) construct makes sense, but do we need that for this hunk?
> @@ -284,7 +285,10 @@ let
> (* Generate the Python binding. *)
> let print_python_binding name { args; optargs; ret; may_set_error } =
> pr "PyObject *\n";
> - pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name;
> + pr "nbd_internal_py_%s (" name;
> + pr_wrap ',' (fun () ->
> + pr "PyObject *self, PyObject *args");
> + pr ")\n";
> pr "{\n";
> pr " PyObject *py_h;\n";
> pr " struct nbd_handle *h;\n";
or is there a shorter way to write that one?
I don't think so (except to write it all on a single line).
The pr_wrap function is a bit weird in that it requires a sub-function
that generates the buffer that it then wraps, rather than just taking
a string parameter directly. Could imagine a pr_wrap_string function
that took the string buffer directly, but that doesn't exist today.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
9:08 p.m.
New subject: [libnbd PATCH v2 5/8] python: Make py_aio_buffer a private struct
Instead of having methods.c poking into the internals of a struct that
we defined, have it use PyMemoryView* and Py_buffer*; this separation
makes it easier to separate how we store the persistent data (for now,
in a PyCapsule) from how we access the data. Eventually, the use of
PyMemoryView* will make it easier for future patches to get rid of
some layers of copying. nbd_internal_py_init_aio_buffer() is new; for
now it doesn't fail, but it can in a future patch.
For now, we still have to hang on to a python reference to the
PyCapsule object holding our malloc'd C pointer, and since the
MemoryView we just created is not backed by a Python object, we must
ensure we don't leak it to Python code. But this will change in an
upcoming patch.
Generated code changes as follows:
| --- python/methods.h.bak 2022-06-06 16:26:40.363250044 -0500
| +++ python/methods.h 2022-06-06 16:37:13.625328491 -0500
| @@ -28,17 +28,12 @@
|
| #include <assert.h>
|
| -struct py_aio_buffer {
| - Py_ssize_t len;
| - void *data;
| - bool initialized;
| -};
| -
| extern char **nbd_internal_py_get_string_list (PyObject *);
| extern void nbd_internal_py_free_string_list (char **);
| extern int nbd_internal_py_get_sockaddr (PyObject *,
| struct sockaddr_storage *, socklen_t *);
| -extern struct py_aio_buffer *nbd_internal_py_get_aio_buffer (PyObject *);
| +extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
| +extern int nbd_internal_py_init_aio_buffer (PyObject *);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-06 16:26:40.360250039 -0500
| +++ python/methods.c 2022-06-06 16:37:13.640328513 -0500
| @@ -3080,7 +3080,8 @@ nbd_internal_py_aio_pread (PyObject *sel
| int64_t ret;
| PyObject *py_ret = NULL;
| PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| - struct py_aio_buffer *buf_buf;
| + PyObject *buf_view = NULL; /* PyMemoryView of buf */
| + Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *completion_user_data = NULL;
| @@ -3110,16 +3111,17 @@ nbd_internal_py_aio_pread (PyObject *sel
| else
| completion.callback = NULL; /* we're not going to call it */
| flags_u32 = flags;
| - buf_buf = nbd_internal_py_get_aio_buffer (buf);
| - if (!buf_buf) goto out;
| + buf_view = nbd_internal_py_get_aio_view (buf, false);
| + if (!buf_view) goto out;
| + py_buf = PyMemoryView_GET_BUFFER (buf_view);
| /* Increment refcount since buffer may be saved by libnbd. */
| Py_INCREF (buf);
| completion_user_data->buf = buf;
| offset_u64 = offset;
|
| - buf_buf->initialized = true;
| - ret = nbd_aio_pread (h, buf_buf->data, buf_buf->len, offset_u64,
| - completion, flags_u32);
| + if (nbd_internal_py_init_aio_buffer (buf) < 0) goto out;
| + ret = nbd_aio_pread (h, py_buf->buf, py_buf->len, offset_u64, completion,
| + flags_u32);
| completion_user_data = NULL;
| if (ret == -1) {
| raise_exception ();
| @@ -3128,6 +3130,7 @@ nbd_internal_py_aio_pread (PyObject *sel
| py_ret = PyLong_FromLongLong (ret);
|
| out:
| + Py_XDECREF (buf_view);
| free_user_data (completion_user_data);
| return py_ret;
| }
| @@ -3140,7 +3143,8 @@ nbd_internal_py_aio_pread_structured (Py
| int64_t ret;
| PyObject *py_ret = NULL;
| PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| - struct py_aio_buffer *buf_buf;
| + PyObject *buf_view = NULL; /* PyMemoryView of buf */
| + Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *chunk_user_data = NULL;
| @@ -3175,8 +3179,9 @@ nbd_internal_py_aio_pread_structured (Py
| else
| completion.callback = NULL; /* we're not going to call it */
| flags_u32 = flags;
| - buf_buf = nbd_internal_py_get_aio_buffer (buf);
| - if (!buf_buf) goto out;
| + buf_view = nbd_internal_py_get_aio_view (buf, false);
| + if (!buf_view) goto out;
| + py_buf = PyMemoryView_GET_BUFFER (buf_view);
| /* Increment refcount since buffer may be saved by libnbd. */
| Py_INCREF (buf);
| completion_user_data->buf = buf;
| @@ -3192,9 +3197,9 @@ nbd_internal_py_aio_pread_structured (Py
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
|
| - buf_buf->initialized = true;
| - ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len,
| - offset_u64, chunk, completion, flags_u32);
| + if (nbd_internal_py_init_aio_buffer (buf) < 0) goto out;
| + ret = nbd_aio_pread_structured (h, py_buf->buf, py_buf->len, offset_u64,
| + chunk, completion, flags_u32);
| chunk_user_data = NULL;
| completion_user_data = NULL;
| if (ret == -1) {
| @@ -3204,6 +3209,7 @@ nbd_internal_py_aio_pread_structured (Py
| py_ret = PyLong_FromLongLong (ret);
|
| out:
| + Py_XDECREF (buf_view);
| free_user_data (chunk_user_data);
| free_user_data (completion_user_data);
| return py_ret;
| @@ -3217,7 +3223,8 @@ nbd_internal_py_aio_pwrite (PyObject *se
| int64_t ret;
| PyObject *py_ret = NULL;
| PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| - struct py_aio_buffer *buf_buf;
| + PyObject *buf_view = NULL; /* PyMemoryView of buf */
| + Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *completion_user_data = NULL;
| @@ -3247,19 +3254,16 @@ nbd_internal_py_aio_pwrite (PyObject *se
| else
| completion.callback = NULL; /* we're not going to call it */
| flags_u32 = flags;
| - buf_buf = nbd_internal_py_get_aio_buffer (buf);
| - if (!buf_buf) goto out;
| + buf_view = nbd_internal_py_get_aio_view (buf, true);
| + if (!buf_view) goto out;
| + py_buf = PyMemoryView_GET_BUFFER (buf_view);
| /* Increment refcount since buffer may be saved by libnbd. */
| Py_INCREF (buf);
| completion_user_data->buf = buf;
| offset_u64 = offset;
|
| - if (!buf_buf->initialized) {
| - memset (buf_buf->data, 0, buf_buf->len);
| - buf_buf->initialized = true;
| - }
| - ret = nbd_aio_pwrite (h, buf_buf->data, buf_buf->len, offset_u64,
| - completion, flags_u32);
| + ret = nbd_aio_pwrite (h, py_buf->buf, py_buf->len, offset_u64, completion,
| + flags_u32);
| completion_user_data = NULL;
| if (ret == -1) {
| raise_exception ();
| @@ -3268,6 +3272,7 @@ nbd_internal_py_aio_pwrite (PyObject *se
| py_ret = PyLong_FromLongLong (ret);
|
| out:
| + Py_XDECREF (buf_view);
| free_user_data (completion_user_data);
| return py_ret;
| }
---
generator/Python.ml | 38 ++++++++++++++++----------------
python/handle.c | 53 ++++++++++++++++++++++++++++++++++++---------
2 files changed, 62 insertions(+), 29 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 6244c8e..1afe0cf 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -34,17 +34,12 @@ let
pr "#include <assert.h>\n";
pr "\n";
pr "\
-struct py_aio_buffer {
- Py_ssize_t len;
- void *data;
- bool initialized;
-};
-
extern char **nbd_internal_py_get_string_list (PyObject *);
extern void nbd_internal_py_free_string_list (char **);
extern int nbd_internal_py_get_sockaddr (PyObject *,
struct sockaddr_storage *, socklen_t *);
-extern struct py_aio_buffer *nbd_internal_py_get_aio_buffer (PyObject *);
+extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
+extern int nbd_internal_py_init_aio_buffer (PyObject *);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -306,7 +301,8 @@ let
| BytesPersistOut (n, _) ->
pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n"
n;
- pr " struct py_aio_buffer *%s_buf;\n" n
+ pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n;
+ pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n
| Closure { cbname } ->
pr " struct user_data *%s_user_data = NULL;\n" cbname;
pr " PyObject *py_%s_fn;\n" cbname;
@@ -366,7 +362,7 @@ let
"n", sprintf "&%s" count,
sprintf "PyByteArray_AS_STRING (%s), %s" n count
| BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
- "O", sprintf "&%s" n, sprintf "%s_buf->data,
%s_buf->len" n n
+ "O", sprintf "&%s" n, sprintf "py_%s->buf,
py_%s->len" n n
| Closure { cbname } -> "O", sprintf "&py_%s_fn" cbname,
cbname
| Enum (n, _) -> "i", sprintf "&%s" n, n
| Flags (n, _) -> "I", sprintf "&%s" n, sprintf
"%s_u32" n
@@ -435,9 +431,17 @@ let
| BytesOut (n, count) ->
pr " %s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count;
pr " if (%s == NULL) goto out;\n" n
- | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
- pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n;
- pr " if (!%s_buf) goto out;\n" n;
+ | BytesPersistIn (n, _) ->
+ pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n;
+ pr " if (!%s_view) goto out;\n" n;
+ pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
+ pr " /* Increment refcount since buffer may be saved by libnbd. */\n";
+ pr " Py_INCREF (%s);\n" n;
+ pr " completion_user_data->buf = %s;\n" n
+ | BytesPersistOut (n, _) ->
+ pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n;
+ pr " if (!%s_view) goto out;\n" n;
+ pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n;
pr " /* Increment refcount since buffer may be saved by libnbd. */\n";
pr " Py_INCREF (%s);\n" n;
pr " completion_user_data->buf = %s;\n" n
@@ -477,13 +481,8 @@ let
(* Second pass, and call the underlying C function. *)
List.iter (
function
- | BytesPersistIn (n, _) ->
- pr " if (!%s_buf->initialized) {\n" n;
- pr " memset (%s_buf->data, 0, %s_buf->len);\n" n n;
- pr " %s_buf->initialized = true;\n" n;
- pr " }\n"
| BytesPersistOut (n, _) ->
- pr " %s_buf->initialized = true;\n" n
+ pr " if (nbd_internal_py_init_aio_buffer (%s) < 0) goto out;\n" n
| _ -> ()
) args;
pr " ret = nbd_%s (" name;
@@ -550,7 +549,8 @@ let
pr " if (%s.obj)\n" n;
pr " PyBuffer_Release (&%s);\n" n
| BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n
- | BytesPersistIn _ | BytesPersistOut _ -> ()
+ | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
+ pr " Py_XDECREF (%s_view);\n" n
| Closure { cbname } ->
pr " free_user_data (%s_user_data);\n" cbname
| Enum _ -> ()
diff --git a/python/handle.c b/python/handle.c
index d42a563..61dd736 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -40,6 +40,12 @@
#include "methods.h"
+struct py_aio_buffer {
+ Py_ssize_t len;
+ void *data;
+ bool initialized;
+};
+
static inline PyObject *
put_handle (struct nbd_handle *h)
{
@@ -98,12 +104,41 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args)
static const char aio_buffer_name[] = "nbd.Buffer";
-struct py_aio_buffer *
+/* Return internal buffer pointer of nbd.Buffer */
+static struct py_aio_buffer *
nbd_internal_py_get_aio_buffer (PyObject *capsule)
{
return PyCapsule_GetPointer (capsule, aio_buffer_name);
}
+/* Return new reference to MemoryView wrapping aio_buffer contents */
+PyObject *
+nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init)
+{
+ struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule);
+
+ if (!buf)
+ return NULL;
+
+ if (require_init && !buf->initialized) {
+ memset (buf->data, 0, buf->len);
+ buf->initialized = true;
+ }
+
+ return PyMemoryView_FromMemory (buf->data, buf->len,
+ require_init ? PyBUF_READ : PyBUF_WRITE);
+}
+
+int
+nbd_internal_py_init_aio_buffer (PyObject *capsule)
+{
+ struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule);
+
+ assert (buf);
+ buf->initialized = true;
+ return 0;
+}
+
static void
free_aio_buffer (PyObject *capsule)
{
@@ -219,23 +254,21 @@ PyObject *
nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args)
{
PyObject *obj;
- struct py_aio_buffer *buf;
+ PyObject *view;
+ PyObject *ret;
if (!PyArg_ParseTuple (args,
"O:nbd_internal_py_aio_buffer_to_bytearray",
&obj))
return NULL;
- buf = nbd_internal_py_get_aio_buffer (obj);
- if (buf == NULL)
+ view = nbd_internal_py_get_aio_view (obj, true);
+ if (view == NULL)
return NULL;
- if (!buf->initialized) {
- memset (buf->data, 0, buf->len);
- buf->initialized = true;
- }
-
- return PyByteArray_FromStringAndSize (buf->data, buf->len);
+ ret = PyByteArray_FromObject (view);
+ Py_DECREF (view);
+ return ret;
}
PyObject *
--
2.36.1
8:17 a.m.
New subject: [libnbd PATCH v2 5/8] python: Make py_aio_buffer a private struct
On Mon, Jun 06, 2022 at 09:08:30PM -0500, Eric Blake wrote:
Instead of having methods.c poking into the internals of a struct
that
we defined, have it use PyMemoryView* and Py_buffer*; this separation
makes it easier to separate how we store the persistent data (for now,
in a PyCapsule) from how we access the data. Eventually, the use of
PyMemoryView* will make it easier for future patches to get rid of
some layers of copying. nbd_internal_py_init_aio_buffer() is new; for
now it doesn't fail, but it can in a future patch.
For now, we still have to hang on to a python reference to the
PyCapsule object holding our malloc'd C pointer, and since the
MemoryView we just created is not backed by a Python object, we must
ensure we don't leak it to Python code. But this will change in an
upcoming patch.
[...]
@@ -477,13 +481,8 @@ let
(* Second pass, and call the underlying C function. *)
List.iter (
function
- | BytesPersistIn (n, _) ->
- pr " if (!%s_buf->initialized) {\n" n;
- pr " memset (%s_buf->data, 0, %s_buf->len);\n" n n;
- pr " %s_buf->initialized = true;\n" n;
- pr " }\n"
| BytesPersistOut (n, _) ->
- pr " %s_buf->initialized = true;\n" n
+ pr " if (nbd_internal_py_init_aio_buffer (%s) < 0) goto out;\n" n
Can't fail, but that was noted in the commit message so that's OK.
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
9:08 p.m.
New subject: [libnbd PATCH v2 6/8] python: Don't unwrap nbd.Buffer in nbd.py
Prior to this commit, we were unwrapping buf._o in nbd.py, which
causes cryptic errors when the user passes in the wrong type:
$ nbdkit -U - memory 10 --run \
'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"'
Traceback (most recent call last):
...
File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread
return libnbdmod.aio_pread(self._o, buf._o, offset, completion,
AttributeError: 'bytearray' object has no attribute '_o'
or worse, if the user passes in an unrelated type that does have an
attribute "_o" but is not a PyCapsule wrapper:
$ nbdkit -U - memory 10 --run 'nbdsh -u "$uri" -c "
class foo(object):
def __init__(self):
self._o = 1
h.aio_pread(foo(), 0)
"'
Traceback (most recent call last):
...
File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread
return libnbdmod.aio_pread(self._o, buf._o, offset, completion,
ValueError: PyCapsule_GetPointer called with invalid PyCapsule object
Change things to instead do the unwrapping in our helper function.
This will also make it easier for an upcoming patch to accept more
types than just nbd.Buffer, with the goal of accepting any python
buffer-like object. For now, passing in the wrong type still fails,
but with a nicer message:
$ ./run nbdkit -U - memory 10 --run \
'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"'
Traceback (most recent call last):
...
File "/home/eblake/libnbd/python/nbd.py", line 2204, in aio_pread
return libnbdmod.aio_pread(self._o, buf, offset, completion, flags)
TypeError: aio_buffer: expecting nbd.Buffer instance
The next patch will clean up the OCaml code, now that none of the
parameter types need alternative text. The generated code changes as
follows:
| --- python/methods.h.bak 2022-06-06 16:55:27.549200523 -0500
| +++ python/methods.h 2022-06-06 16:55:34.097211464 -0500
| @@ -34,6 +34,7 @@
| struct sockaddr_storage *, socklen_t *);
| extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
| extern int nbd_internal_py_init_aio_buffer (PyObject *);
| +extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-06 16:55:27.543200513 -0500
| +++ python/methods.c 2022-06-06 16:55:34.114211492 -0500
| @@ -3079,7 +3079,7 @@ nbd_internal_py_aio_pread (PyObject *sel
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| + PyObject *buf; /* Instance of nbd.Buffer */
| PyObject *buf_view = NULL; /* PyMemoryView of buf */
| Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| @@ -3142,7 +3142,7 @@ nbd_internal_py_aio_pread_structured (Py
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| + PyObject *buf; /* Instance of nbd.Buffer */
| PyObject *buf_view = NULL; /* PyMemoryView of buf */
| Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| @@ -3222,7 +3222,7 @@ nbd_internal_py_aio_pwrite (PyObject *se
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| + PyObject *buf; /* Instance of nbd.Buffer */
| PyObject *buf_view = NULL; /* PyMemoryView of buf */
| Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| --- python/nbd.py.bak 2022-06-06 16:55:27.552200528 -0500
| +++ python/nbd.py 2022-06-06 16:55:34.123211507 -0500
| @@ -141,7 +141,7 @@ class Buffer(object):
|
| def to_bytearray(self):
| '''Copy an AIO buffer into a bytearray.'''
| - return libnbdmod.aio_buffer_to_bytearray(self._o)
| + return libnbdmod.aio_buffer_to_bytearray(self)
|
| def size(self):
| '''Return the size of an AIO buffer.'''
| @@ -161,7 +161,7 @@ class Buffer(object):
| always returns true. If size > 0, we check the interval
| [offset..offset+size-1].
| '''
| - return libnbdmod.aio_buffer_is_zero(self._o, offset, size)
| + return libnbdmod.aio_buffer_is_zero(self, offset, size)
|
|
| class NBD(object):
| @@ -2208,8 +2208,7 @@ class NBD(object):
| alter which scenarios should await a server reply rather
| than failing fast.
| '''
| - return libnbdmod.aio_pread(self._o, buf._o, offset, completion,
| - flags)
| + return libnbdmod.aio_pread(self._o, buf, offset, completion, flags)
|
| def aio_pread_structured(self, buf, offset, chunk, completion=None,
| flags=0):
| @@ -2243,8 +2242,8 @@ class NBD(object):
| alter which scenarios should await a server reply rather
| than failing fast.
| '''
| - return libnbdmod.aio_pread_structured(self._o, buf._o, offset,
| - chunk, completion, flags)
| + return libnbdmod.aio_pread_structured(self._o, buf, offset, chunk,
| + completion, flags)
|
| def aio_pwrite(self, buf, offset, completion=None, flags=0):
| u'''â–¶ write to the NBD server
| @@ -2267,8 +2266,7 @@ class NBD(object):
| alter which scenarios should await a server reply rather
| than failing fast.
| '''
| - return libnbdmod.aio_pwrite(self._o, buf._o, offset, completion,
| - flags)
| + return libnbdmod.aio_pwrite(self._o, buf, offset, completion, flags)
|
| def aio_disconnect(self, flags=0):
| u'''â–¶ disconnect from the NBD server
---
generator/Python.ml | 14 +++++++-------
python/handle.c | 20 ++++++++++++++------
python/utils.c | 20 +++++++++++++++++++-
3 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 1afe0cf..101f3e0 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -40,6 +40,7 @@ let
struct sockaddr_storage *, socklen_t *);
extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
extern int nbd_internal_py_init_aio_buffer (PyObject *);
+extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -299,8 +300,7 @@ let
pr " Py_ssize_t %s;\n" count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) ->
- pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n"
- n;
+ pr " PyObject *%s; /* Instance of nbd.Buffer */\n" n;
pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n;
pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n
| Closure { cbname } ->
@@ -755,11 +755,11 @@ let
def to_bytearray(self):
'''Copy an AIO buffer into a bytearray.'''
- return libnbdmod.aio_buffer_to_bytearray(self._o)
+ return libnbdmod.aio_buffer_to_bytearray(self)
def size(self):
'''Return the size of an AIO buffer.'''
- return libnbdmod.aio_buffer_size(self._o)
+ return libnbdmod.aio_buffer_size(self)
def is_zero(self, offset=0, size=-1):
'''Returns true if and only if all bytes in the buffer are zeroes.
@@ -775,7 +775,7 @@ let
always returns true. If size > 0, we check the interval
[offset..offset+size-1].
'''
- return libnbdmod.aio_buffer_is_zero(self._o, offset, size)
+ return libnbdmod.aio_buffer_is_zero(self, offset, size)
class NBD(object):
@@ -800,8 +800,8 @@ let
| Bool n -> n, None, None
| BytesIn (n, _) -> n, None, None
| BytesOut (_, count) -> count, None, None
- | BytesPersistIn (n, _) -> n, None, Some (sprintf "%s._o" n)
- | BytesPersistOut (n, _) -> n, None, Some (sprintf "%s._o" n)
+ | BytesPersistIn (n, _) -> n, None, None
+ | BytesPersistOut (n, _) -> n, None, None
| Closure { cbname } -> cbname, None, None
| Enum (n, _) -> n, None, None
| Flags (n, _) -> n, None, None
diff --git a/python/handle.c b/python/handle.c
index 61dd736..79b369d 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -106,16 +106,24 @@ static const char aio_buffer_name[] = "nbd.Buffer";
/* Return internal buffer pointer of nbd.Buffer */
static struct py_aio_buffer *
-nbd_internal_py_get_aio_buffer (PyObject *capsule)
+nbd_internal_py_get_aio_buffer (PyObject *object)
{
- return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) {
+ PyObject *capsule = PyObject_GetAttrString(object, "_o");
+
+ return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ }
+
+ PyErr_SetString (PyExc_TypeError,
+ "aio_buffer: expecting nbd.Buffer instance");
+ return NULL;
}
/* Return new reference to MemoryView wrapping aio_buffer contents */
PyObject *
-nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init)
+nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule);
+ struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
if (!buf)
return NULL;
@@ -130,9 +138,9 @@ nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init)
}
int
-nbd_internal_py_init_aio_buffer (PyObject *capsule)
+nbd_internal_py_init_aio_buffer (PyObject *object)
{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule);
+ struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
assert (buf);
buf->initialized = true;
diff --git a/python/utils.c b/python/utils.c
index 37f0c55..e0df181 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -155,3 +155,21 @@ nbd_internal_py_get_sockaddr (PyObject *addr,
return -1;
}
}
+
+/* Obtain the type object for nbd.Buffer */
+PyObject *
+nbd_internal_py_get_nbd_buffer_type (void)
+{
+ static PyObject *type;
+
+ if (!type) {
+ PyObject *modname = PyUnicode_FromString ("nbd");
+ PyObject *module = PyImport_Import (modname);
+ assert (module);
+ type = PyObject_GetAttrString (module, "Buffer");
+ assert (type);
+ Py_DECREF (modname);
+ Py_DECREF (module);
+ }
+ return type;
+}
--
2.36.1
8:18 a.m.
New subject: [libnbd PATCH v2 6/8] python: Don't unwrap nbd.Buffer in nbd.py
On Mon, Jun 06, 2022 at 09:08:31PM -0500, Eric Blake wrote:
Prior to this commit, we were unwrapping buf._o in nbd.py, which
causes cryptic errors when the user passes in the wrong type:
$ nbdkit -U - memory 10 --run \
'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"'
Traceback (most recent call last):
...
File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread
return libnbdmod.aio_pread(self._o, buf._o, offset, completion,
AttributeError: 'bytearray' object has no attribute '_o'
or worse, if the user passes in an unrelated type that does have an
attribute "_o" but is not a PyCapsule wrapper:
$ nbdkit -U - memory 10 --run 'nbdsh -u "$uri" -c "
class foo(object):
def __init__(self):
self._o = 1
h.aio_pread(foo(), 0)
"'
Traceback (most recent call last):
...
File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread
return libnbdmod.aio_pread(self._o, buf._o, offset, completion,
ValueError: PyCapsule_GetPointer called with invalid PyCapsule object
Change things to instead do the unwrapping in our helper function.
This will also make it easier for an upcoming patch to accept more
types than just nbd.Buffer, with the goal of accepting any python
buffer-like object. For now, passing in the wrong type still fails,
but with a nicer message:
$ ./run nbdkit -U - memory 10 --run \
'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"'
Traceback (most recent call last):
...
File "/home/eblake/libnbd/python/nbd.py", line 2204, in aio_pread
return libnbdmod.aio_pread(self._o, buf, offset, completion, flags)
TypeError: aio_buffer: expecting nbd.Buffer instance
The next patch will clean up the OCaml code, now that none of the
parameter types need alternative text. The generated code changes as
follows:
| --- python/methods.h.bak 2022-06-06 16:55:27.549200523 -0500
| +++ python/methods.h 2022-06-06 16:55:34.097211464 -0500
| @@ -34,6 +34,7 @@
| struct sockaddr_storage *, socklen_t *);
| extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
| extern int nbd_internal_py_init_aio_buffer (PyObject *);
| +extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-06 16:55:27.543200513 -0500
| +++ python/methods.c 2022-06-06 16:55:34.114211492 -0500
| @@ -3079,7 +3079,7 @@ nbd_internal_py_aio_pread (PyObject *sel
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| + PyObject *buf; /* Instance of nbd.Buffer */
| PyObject *buf_view = NULL; /* PyMemoryView of buf */
| Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| @@ -3142,7 +3142,7 @@ nbd_internal_py_aio_pread_structured (Py
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| + PyObject *buf; /* Instance of nbd.Buffer */
| PyObject *buf_view = NULL; /* PyMemoryView of buf */
| Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| @@ -3222,7 +3222,7 @@ nbd_internal_py_aio_pwrite (PyObject *se
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| + PyObject *buf; /* Instance of nbd.Buffer */
| PyObject *buf_view = NULL; /* PyMemoryView of buf */
| Py_buffer *py_buf; /* buffer of buf_view */
| uint64_t offset_u64;
| --- python/nbd.py.bak 2022-06-06 16:55:27.552200528 -0500
| +++ python/nbd.py 2022-06-06 16:55:34.123211507 -0500
| @@ -141,7 +141,7 @@ class Buffer(object):
|
| def to_bytearray(self):
| '''Copy an AIO buffer into a bytearray.'''
| - return libnbdmod.aio_buffer_to_bytearray(self._o)
| + return libnbdmod.aio_buffer_to_bytearray(self)
|
| def size(self):
| '''Return the size of an AIO buffer.'''
| @@ -161,7 +161,7 @@ class Buffer(object):
| always returns true. If size > 0, we check the interval
| [offset..offset+size-1].
| '''
| - return libnbdmod.aio_buffer_is_zero(self._o, offset, size)
| + return libnbdmod.aio_buffer_is_zero(self, offset, size)
|
|
| class NBD(object):
| @@ -2208,8 +2208,7 @@ class NBD(object):
| alter which scenarios should await a server reply rather
| than failing fast.
| '''
| - return libnbdmod.aio_pread(self._o, buf._o, offset, completion,
| - flags)
| + return libnbdmod.aio_pread(self._o, buf, offset, completion, flags)
|
| def aio_pread_structured(self, buf, offset, chunk, completion=None,
| flags=0):
| @@ -2243,8 +2242,8 @@ class NBD(object):
| alter which scenarios should await a server reply rather
| than failing fast.
| '''
| - return libnbdmod.aio_pread_structured(self._o, buf._o, offset,
| - chunk, completion, flags)
| + return libnbdmod.aio_pread_structured(self._o, buf, offset, chunk,
| + completion, flags)
|
| def aio_pwrite(self, buf, offset, completion=None, flags=0):
| u'''â–¶ write to the NBD server
| @@ -2267,8 +2266,7 @@ class NBD(object):
| alter which scenarios should await a server reply rather
| than failing fast.
| '''
| - return libnbdmod.aio_pwrite(self._o, buf._o, offset, completion,
| - flags)
| + return libnbdmod.aio_pwrite(self._o, buf, offset, completion, flags)
|
| def aio_disconnect(self, flags=0):
| u'''â–¶ disconnect from the NBD server
---
generator/Python.ml | 14 +++++++-------
python/handle.c | 20 ++++++++++++++------
python/utils.c | 20 +++++++++++++++++++-
3 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 1afe0cf..101f3e0 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -40,6 +40,7 @@ let
struct sockaddr_storage *, socklen_t *);
extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
extern int nbd_internal_py_init_aio_buffer (PyObject *);
+extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -299,8 +300,7 @@ let
pr " Py_ssize_t %s;\n" count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) ->
- pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer
*/\n"
- n;
+ pr " PyObject *%s; /* Instance of nbd.Buffer */\n" n;
pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n;
pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n
| Closure { cbname } ->
@@ -755,11 +755,11 @@ let
def to_bytearray(self):
'''Copy an AIO buffer into a bytearray.'''
- return libnbdmod.aio_buffer_to_bytearray(self._o)
+ return libnbdmod.aio_buffer_to_bytearray(self)
def size(self):
'''Return the size of an AIO buffer.'''
- return libnbdmod.aio_buffer_size(self._o)
+ return libnbdmod.aio_buffer_size(self)
def is_zero(self, offset=0, size=-1):
'''Returns true if and only if all bytes in the buffer are zeroes.
@@ -775,7 +775,7 @@ let
always returns true. If size > 0, we check the interval
[offset..offset+size-1].
'''
- return libnbdmod.aio_buffer_is_zero(self._o, offset, size)
+ return libnbdmod.aio_buffer_is_zero(self, offset, size)
class NBD(object):
@@ -800,8 +800,8 @@ let
| Bool n -> n, None, None
| BytesIn (n, _) -> n, None, None
| BytesOut (_, count) -> count, None, None
- | BytesPersistIn (n, _) -> n, None, Some (sprintf "%s._o" n)
- | BytesPersistOut (n, _) -> n, None, Some (sprintf "%s._o" n)
+ | BytesPersistIn (n, _) -> n, None, None
+ | BytesPersistOut (n, _) -> n, None, None
| Closure { cbname } -> cbname, None, None
| Enum (n, _) -> n, None, None
| Flags (n, _) -> n, None, None
diff --git a/python/handle.c b/python/handle.c
index 61dd736..79b369d 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -106,16 +106,24 @@ static const char aio_buffer_name[] = "nbd.Buffer";
/* Return internal buffer pointer of nbd.Buffer */
static struct py_aio_buffer *
-nbd_internal_py_get_aio_buffer (PyObject *capsule)
+nbd_internal_py_get_aio_buffer (PyObject *object)
{
- return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) {
+ PyObject *capsule = PyObject_GetAttrString(object, "_o");
+
+ return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ }
+
+ PyErr_SetString (PyExc_TypeError,
+ "aio_buffer: expecting nbd.Buffer instance");
+ return NULL;
}
/* Return new reference to MemoryView wrapping aio_buffer contents */
PyObject *
-nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init)
+nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule);
+ struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
if (!buf)
return NULL;
@@ -130,9 +138,9 @@ nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init)
}
int
-nbd_internal_py_init_aio_buffer (PyObject *capsule)
+nbd_internal_py_init_aio_buffer (PyObject *object)
{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule);
+ struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
assert (buf);
buf->initialized = true;
diff --git a/python/utils.c b/python/utils.c
index 37f0c55..e0df181 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -155,3 +155,21 @@ nbd_internal_py_get_sockaddr (PyObject *addr,
return -1;
}
}
+
+/* Obtain the type object for nbd.Buffer */
+PyObject *
+nbd_internal_py_get_nbd_buffer_type (void)
+{
+ static PyObject *type;
+
+ if (!type) {
+ PyObject *modname = PyUnicode_FromString ("nbd");
+ PyObject *module = PyImport_Import (modname);
+ assert (module);
+ type = PyObject_GetAttrString (module, "Buffer");
+ assert (type);
+ Py_DECREF (modname);
+ Py_DECREF (module);
+ }
+ return type;
+}
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
9:08 p.m.
New subject: [libnbd PATCH v2 7/8] python: Simplify python generator
Now that none of our parameter types uses a getter sequence, we can
simplify the code for generating nbd.py. No change to generated
output.
---
generator/Python.ml | 49 ++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 101f3e0..c49af4f 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -797,31 +797,31 @@ let
let args =
List.map (
function
- | Bool n -> n, None, None
- | BytesIn (n, _) -> n, None, None
- | BytesOut (_, count) -> count, None, None
- | BytesPersistIn (n, _) -> n, None, None
- | BytesPersistOut (n, _) -> n, None, None
- | Closure { cbname } -> cbname, None, None
- | Enum (n, _) -> n, None, None
- | Flags (n, _) -> n, None, None
- | Fd n | Int n -> n, None, None
- | Int64 n -> n, None, None
- | Path n -> n, None, None
- | SizeT n -> n, None, None
- | SockAddrAndLen (n, _) -> n, None, None
- | String n -> n, None, None
- | StringList n -> n, None, None
- | UInt n -> n, None, None
- | UInt32 n -> n, None, None
- | UInt64 n -> n, None, None
- | UIntPtr n -> n, None, None
+ | Bool n -> n, None
+ | BytesIn (n, _) -> n, None
+ | BytesOut (_, count) -> count, None
+ | BytesPersistIn (n, _) -> n, None
+ | BytesPersistOut (n, _) -> n, None
+ | Closure { cbname } -> cbname, None
+ | Enum (n, _) -> n, None
+ | Flags (n, _) -> n, None
+ | Fd n | Int n -> n, None
+ | Int64 n -> n, None
+ | Path n -> n, None
+ | SizeT n -> n, None
+ | SockAddrAndLen (n, _) -> n, None
+ | String n -> n, None
+ | StringList n -> n, None
+ | UInt n -> n, None
+ | UInt32 n -> n, None
+ | UInt64 n -> n, None
+ | UIntPtr n -> n, None
) args in
let optargs =
List.map (
function
- | OClosure { cbname } -> cbname, Some "None", None
- | OFlags (n, _, _) -> n, Some "0", None
+ | OClosure { cbname } -> cbname, Some "None"
+ | OFlags (n, _, _) -> n, Some "0"
) optargs in
let args = args @ optargs in
pr " def %s(" name;
@@ -829,8 +829,8 @@ let
pr "self";
List.iter (
function
- | n, None, _ -> pr ", %s" n
- | n, Some default, _ -> pr ", %s=%s" n default
+ | n, None -> pr ", %s" n
+ | n, Some default -> pr ", %s=%s" n default
) args);
pr "):\n";
let longdesc = Str.global_replace py_fn_rex "C<nbd.\\1>" longdesc
in
@@ -842,8 +842,7 @@ let
pr "self._o";
List.iter (
function
- | _, _, Some getter -> pr ", %s" getter
- | n, _, None -> pr ", %s" n
+ | n, _ -> pr ", %s" n
) args);
pr ")\n";
pr "\n"
--
2.36.1
8:19 a.m.
New subject: [libnbd PATCH v2 7/8] python: Simplify python generator
On Mon, Jun 06, 2022 at 09:08:32PM -0500, Eric Blake wrote:
Now that none of our parameter types uses a getter sequence, we can
simplify the code for generating nbd.py. No change to generated
output.
---
generator/Python.ml | 49 ++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 101f3e0..c49af4f 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -797,31 +797,31 @@ let
let args =
List.map (
function
- | Bool n -> n, None, None
- | BytesIn (n, _) -> n, None, None
- | BytesOut (_, count) -> count, None, None
- | BytesPersistIn (n, _) -> n, None, None
- | BytesPersistOut (n, _) -> n, None, None
- | Closure { cbname } -> cbname, None, None
- | Enum (n, _) -> n, None, None
- | Flags (n, _) -> n, None, None
- | Fd n | Int n -> n, None, None
- | Int64 n -> n, None, None
- | Path n -> n, None, None
- | SizeT n -> n, None, None
- | SockAddrAndLen (n, _) -> n, None, None
- | String n -> n, None, None
- | StringList n -> n, None, None
- | UInt n -> n, None, None
- | UInt32 n -> n, None, None
- | UInt64 n -> n, None, None
- | UIntPtr n -> n, None, None
+ | Bool n -> n, None
+ | BytesIn (n, _) -> n, None
+ | BytesOut (_, count) -> count, None
+ | BytesPersistIn (n, _) -> n, None
+ | BytesPersistOut (n, _) -> n, None
+ | Closure { cbname } -> cbname, None
+ | Enum (n, _) -> n, None
+ | Flags (n, _) -> n, None
+ | Fd n | Int n -> n, None
+ | Int64 n -> n, None
+ | Path n -> n, None
+ | SizeT n -> n, None
+ | SockAddrAndLen (n, _) -> n, None
+ | String n -> n, None
+ | StringList n -> n, None
+ | UInt n -> n, None
+ | UInt32 n -> n, None
+ | UInt64 n -> n, None
+ | UIntPtr n -> n, None
) args in
let optargs =
List.map (
function
- | OClosure { cbname } -> cbname, Some "None", None
- | OFlags (n, _, _) -> n, Some "0", None
+ | OClosure { cbname } -> cbname, Some "None"
+ | OFlags (n, _, _) -> n, Some "0"
) optargs in
let args = args @ optargs in
pr " def %s(" name;
@@ -829,8 +829,8 @@ let
pr "self";
List.iter (
function
- | n, None, _ -> pr ", %s" n
- | n, Some default, _ -> pr ", %s=%s" n default
+ | n, None -> pr ", %s" n
+ | n, Some default -> pr ", %s=%s" n default
) args);
pr "):\n";
let longdesc = Str.global_replace py_fn_rex "C<nbd.\\1>" longdesc
in
@@ -842,8 +842,7 @@ let
pr "self._o";
List.iter (
function
- | _, _, Some getter -> pr ", %s" getter
- | n, _, None -> pr ", %s" n
+ | n, _ -> pr ", %s" n
) args);
pr ")\n";
pr "\n"
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
9:08 p.m.
New subject: [libnbd PATCH v2 8/8] python: Make nbd.Buffer lighter-weight
Instead of storing a PyCapsule in _o and repeatedly doing lookups to
dereference a stored malloc'd pointer, it is easier to just directly
store a Python buffer-like object as _o. Track initialization via
._init: absent for nbd.Buffer(int), present for
nbd.Buffer.from_bytearray or after we have forced initialization due
to aio_pread or .to_bytearray. At this point, we are still copying in
and out of .{from,to}_bytearray, but we are getting closer to reducing
the number of copies. The sheer size in reduced lines of code is a
testament to the ease of doing things closer to Python, rather than
hidden behind unpacking a PyCapsule layer.
Acceptable side effect: nbd.Buffer(-1) changes from:
RuntimeError: length < 0
to
SystemError: Negative size passed to PyByteArray_FromStringAndSize
The generated code changes as follows:
| --- python/methods.h.bak 2022-06-06 20:36:01.973327739 -0500
| +++ python/methods.h 2022-06-06 20:36:16.146352508 -0500
| @@ -62,9 +62,6 @@
| extern PyObject *nbd_internal_py_close (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_display_version (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args);
| -extern PyObject *nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject
*args);
| -extern PyObject *nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject
*args);
| -extern PyObject *nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_set_debug (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_get_debug (PyObject *self, PyObject *args);
| --- python/nbd.py.bak 2022-06-06 20:36:01.978327748 -0500
| +++ python/nbd.py 2022-06-06 20:56:41.866729560 -0500
| @@ -134,18 +134,21 @@ class Buffer(object):
| bytearray constructor. Otherwise, ba is copied. Either way, the
| resulting AIO buffer is independent from the original.
| '''
| - o = libnbdmod.aio_buffer_from_bytearray(ba)
| self = cls(0)
| - self._o = o
| + self._o = bytearray(ba)
| + self._init = True
| return self
|
| def to_bytearray(self):
| '''Copy an AIO buffer into a bytearray.'''
| - return libnbdmod.aio_buffer_to_bytearray(self)
| + if not hasattr(self, '_init'):
| + self._o = bytearray(len(self._o))
| + self._init = True
| + return bytearray(self._o)
|
| def size(self):
| '''Return the size of an AIO buffer.'''
| - return libnbdmod.aio_buffer_size(self)
| + return len(self._o)
|
| def is_zero(self, offset=0, size=-1):
| '''Returns true if and only if all bytes in the buffer are zeroes.
| @@ -161,7 +164,8 @@ class Buffer(object):
| always returns true. If size > 0, we check the interval
| [offset..offset+size-1].
| '''
| - return libnbdmod.aio_buffer_is_zero(self, offset, size)
| + return libnbdmod.aio_buffer_is_zero(self._o, offset, size,
| + hasattr(self, '_init'))
|
|
| class NBD(object):
---
generator/Python.ml | 20 ++--
python/handle.c | 242 +++++++++-----------------------------------
2 files changed, 56 insertions(+), 206 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index c49af4f..86781ac 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -73,9 +73,6 @@ let
) ([ "create"; "close";
"display_version";
"alloc_aio_buffer";
- "aio_buffer_from_bytearray";
- "aio_buffer_to_bytearray";
- "aio_buffer_size";
"aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n";
@@ -105,9 +102,6 @@ let
) ([ "create"; "close";
"display_version";
"alloc_aio_buffer";
- "aio_buffer_from_bytearray";
- "aio_buffer_to_bytearray";
- "aio_buffer_size";
"aio_buffer_is_zero"] @ List.map fst handle_calls);
pr " { NULL, NULL, 0, NULL }\n";
pr "};\n";
@@ -748,18 +742,21 @@ let
bytearray constructor. Otherwise, ba is copied. Either way, the
resulting AIO buffer is independent from the original.
'''
- o = libnbdmod.aio_buffer_from_bytearray(ba)
self = cls(0)
- self._o = o
+ self._o = bytearray(ba)
+ self._init = True
return self
def to_bytearray(self):
'''Copy an AIO buffer into a bytearray.'''
- return libnbdmod.aio_buffer_to_bytearray(self)
+ if not hasattr(self, '_init'):
+ self._o = bytearray(len(self._o))
+ self._init = True
+ return bytearray(self._o)
def size(self):
'''Return the size of an AIO buffer.'''
- return libnbdmod.aio_buffer_size(self)
+ return len(self._o)
def is_zero(self, offset=0, size=-1):
'''Returns true if and only if all bytes in the buffer are zeroes.
@@ -775,7 +772,8 @@ let
always returns true. If size > 0, we check the interval
[offset..offset+size-1].
'''
- return libnbdmod.aio_buffer_is_zero(self, offset, size)
+ return libnbdmod.aio_buffer_is_zero(self._o, offset, size,
+ hasattr(self, '_init'))
class NBD(object):
diff --git a/python/handle.c b/python/handle.c
index 79b369d..ca6c8e9 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -40,12 +40,6 @@
#include "methods.h"
-struct py_aio_buffer {
- Py_ssize_t len;
- void *data;
- bool initialized;
-};
-
static inline PyObject *
put_handle (struct nbd_handle *h)
{
@@ -102,242 +96,100 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args)
return Py_None;
}
-static const char aio_buffer_name[] = "nbd.Buffer";
-
-/* Return internal buffer pointer of nbd.Buffer */
-static struct py_aio_buffer *
-nbd_internal_py_get_aio_buffer (PyObject *object)
+/* Return new reference to MemoryView wrapping aio_buffer contents */
+PyObject *
+nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
{
+ PyObject *buffer = NULL;
+
if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) {
- PyObject *capsule = PyObject_GetAttrString(object, "_o");
+ buffer = PyObject_GetAttrString (object, "_o");
- return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ if (require_init && ! PyObject_HasAttrString (object, "_init")) {
+ assert (PyByteArray_Check (buffer));
+ memset (PyByteArray_AS_STRING (buffer), 0,
+ PyByteArray_GET_SIZE (buffer));
+ if (PyObject_SetAttrString (object, "_init", Py_True) < 0)
+ return NULL;
+ }
}
+ if (buffer)
+ return PyMemoryView_FromObject (buffer);
+
PyErr_SetString (PyExc_TypeError,
"aio_buffer: expecting nbd.Buffer instance");
return NULL;
}
-/* Return new reference to MemoryView wrapping aio_buffer contents */
-PyObject *
-nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
-{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
-
- if (!buf)
- return NULL;
-
- if (require_init && !buf->initialized) {
- memset (buf->data, 0, buf->len);
- buf->initialized = true;
- }
-
- return PyMemoryView_FromMemory (buf->data, buf->len,
- require_init ? PyBUF_READ : PyBUF_WRITE);
-}
-
int
nbd_internal_py_init_aio_buffer (PyObject *object)
{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
-
- assert (buf);
- buf->initialized = true;
+ if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ()))
+ return PyObject_SetAttrString (object, "_init", Py_True);
return 0;
}
-static void
-free_aio_buffer (PyObject *capsule)
-{
- struct py_aio_buffer *buf = PyCapsule_GetPointer (capsule, aio_buffer_name);
-
- if (buf)
- free (buf->data);
- free (buf);
-}
-
-/* Allocate a persistent buffer used for nbd_aio_pread. */
+/* Allocate an uninitialized persistent buffer used for nbd_aio_pread. */
PyObject *
nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args)
{
- struct py_aio_buffer *buf;
- PyObject *ret;
+ Py_ssize_t len;
- buf = malloc (sizeof *buf);
- if (buf == NULL) {
- PyErr_NoMemory ();
- return NULL;
- }
-
- buf->initialized = false;
if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer",
- &buf->len)) {
- free (buf);
+ &len))
return NULL;
- }
- if (buf->len < 0) {
- PyErr_SetString (PyExc_RuntimeError, "length < 0");
- free (buf);
- return NULL;
- }
- buf->data = malloc (buf->len);
- if (buf->data == NULL) {
- PyErr_NoMemory ();
- free (buf);
- return NULL;
- }
-
- ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer);
- if (ret == NULL) {
- free (buf->data);
- free (buf);
- return NULL;
- }
-
- return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args)
-{
- PyObject *obj;
- PyObject *arr = NULL;
- Py_ssize_t len;
- void *data;
- struct py_aio_buffer *buf;
- PyObject *ret;
-
- if (!PyArg_ParseTuple (args,
- "O:nbd_internal_py_aio_buffer_from_bytearray",
- &obj))
- return NULL;
-
- if (! PyByteArray_Check (obj)) {
- arr = PyByteArray_FromObject (obj);
- if (arr == NULL)
- return NULL;
- obj = arr;
- }
- data = PyByteArray_AsString (obj);
- if (!data) {
- PyErr_SetString (PyExc_RuntimeError,
- "parameter is not a bytearray or buffer");
- Py_XDECREF (arr);
- return NULL;
- }
- len = PyByteArray_Size (obj);
-
- buf = malloc (sizeof *buf);
- if (buf == NULL) {
- PyErr_NoMemory ();
- Py_XDECREF (arr);
- return NULL;
- }
-
- buf->len = len;
- buf->data = malloc (len);
- if (buf->data == NULL) {
- PyErr_NoMemory ();
- free (buf);
- Py_XDECREF (arr);
- return NULL;
- }
- memcpy (buf->data, data, len);
- Py_XDECREF (arr);
- buf->initialized = true;
-
- ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer);
- if (ret == NULL) {
- free (buf->data);
- free (buf);
- return NULL;
- }
-
- return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args)
-{
- PyObject *obj;
- PyObject *view;
- PyObject *ret;
-
- if (!PyArg_ParseTuple (args,
- "O:nbd_internal_py_aio_buffer_to_bytearray",
- &obj))
- return NULL;
-
- view = nbd_internal_py_get_aio_view (obj, true);
- if (view == NULL)
- return NULL;
-
- ret = PyByteArray_FromObject (view);
- Py_DECREF (view);
- return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args)
-{
- PyObject *obj;
- struct py_aio_buffer *buf;
-
- if (!PyArg_ParseTuple (args,
- "O:nbd_internal_py_aio_buffer_size",
- &obj))
- return NULL;
-
- buf = nbd_internal_py_get_aio_buffer (obj);
- if (buf == NULL)
- return NULL;
-
- return PyLong_FromSsize_t (buf->len);
+ /* Constructing bytearray(len) in python zeroes the memory; doing it this
+ * way gives uninitialized memory. This correctly flags negative len.
+ */
+ return PyByteArray_FromStringAndSize (NULL, len);
}
PyObject *
nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args)
{
- PyObject *obj;
- struct py_aio_buffer *buf;
+ Py_buffer buf;
Py_ssize_t offset, size;
+ int init;
+ PyObject *ret = NULL;
if (!PyArg_ParseTuple (args,
- "Onn:nbd_internal_py_aio_buffer_is_zero",
- &obj, &offset, &size))
+ "y*nnp:nbd_internal_py_aio_buffer_is_zero",
+ &buf, &offset, &size, &init))
return NULL;
- if (size == 0)
- Py_RETURN_TRUE;
-
- buf = nbd_internal_py_get_aio_buffer (obj);
- if (buf == NULL)
- return NULL;
+ if (size == 0) {
+ ret = Py_True;
+ goto out;
+ }
/* Check the bounds of the offset. */
- if (offset < 0 || offset > buf->len) {
+ if (offset < 0 || offset > buf.len) {
PyErr_SetString (PyExc_IndexError, "offset out of range");
- return NULL;
+ goto out;
}
/* Compute or check the length. */
if (size == -1)
- size = buf->len - offset;
+ size = buf.len - offset;
else if (size < 0) {
PyErr_SetString (PyExc_IndexError,
"size cannot be negative, "
"except -1 to mean to the end of the buffer");
- return NULL;
+ goto out;
}
- else if ((size_t) offset + size > buf->len) {
+ else if ((size_t) offset + size > buf.len) {
PyErr_SetString (PyExc_IndexError, "size out of range");
- return NULL;
+ goto out;
}
- if (!buf->initialized || is_zero (buf->data + offset, size))
- Py_RETURN_TRUE;
+ if (!init || is_zero (buf.buf + offset, size))
+ ret = Py_True;
else
- Py_RETURN_FALSE;
+ ret = Py_False;
+ out:
+ PyBuffer_Release (&buf);
+ Py_XINCREF (ret);
+ return ret;
}
--
2.36.1
8:20 a.m.
New subject: [libnbd PATCH v2 8/8] python: Make nbd.Buffer lighter-weight
On Mon, Jun 06, 2022 at 09:08:33PM -0500, Eric Blake wrote:
Instead of storing a PyCapsule in _o and repeatedly doing lookups to
dereference a stored malloc'd pointer, it is easier to just directly
store a Python buffer-like object as _o. Track initialization via
._init: absent for nbd.Buffer(int), present for
nbd.Buffer.from_bytearray or after we have forced initialization due
to aio_pread or .to_bytearray. At this point, we are still copying in
and out of .{from,to}_bytearray, but we are getting closer to reducing
the number of copies. The sheer size in reduced lines of code is a
testament to the ease of doing things closer to Python, rather than
hidden behind unpacking a PyCapsule layer.
Acceptable side effect: nbd.Buffer(-1) changes from:
RuntimeError: length < 0
to
SystemError: Negative size passed to PyByteArray_FromStringAndSize
The generated code changes as follows:
| --- python/methods.h.bak 2022-06-06 20:36:01.973327739 -0500
| +++ python/methods.h 2022-06-06 20:36:16.146352508 -0500
| @@ -62,9 +62,6 @@
| extern PyObject *nbd_internal_py_close (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_display_version (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args);
| -extern PyObject *nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject
*args);
| -extern PyObject *nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject
*args);
| -extern PyObject *nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_set_debug (PyObject *self, PyObject *args);
| extern PyObject *nbd_internal_py_get_debug (PyObject *self, PyObject *args);
| --- python/nbd.py.bak 2022-06-06 20:36:01.978327748 -0500
| +++ python/nbd.py 2022-06-06 20:56:41.866729560 -0500
| @@ -134,18 +134,21 @@ class Buffer(object):
| bytearray constructor. Otherwise, ba is copied. Either way, the
| resulting AIO buffer is independent from the original.
| '''
| - o = libnbdmod.aio_buffer_from_bytearray(ba)
| self = cls(0)
| - self._o = o
| + self._o = bytearray(ba)
| + self._init = True
| return self
|
| def to_bytearray(self):
| '''Copy an AIO buffer into a bytearray.'''
| - return libnbdmod.aio_buffer_to_bytearray(self)
| + if not hasattr(self, '_init'):
| + self._o = bytearray(len(self._o))
| + self._init = True
| + return bytearray(self._o)
|
| def size(self):
| '''Return the size of an AIO buffer.'''
| - return libnbdmod.aio_buffer_size(self)
| + return len(self._o)
|
| def is_zero(self, offset=0, size=-1):
| '''Returns true if and only if all bytes in the buffer are
zeroes.
| @@ -161,7 +164,8 @@ class Buffer(object):
| always returns true. If size > 0, we check the interval
| [offset..offset+size-1].
| '''
| - return libnbdmod.aio_buffer_is_zero(self, offset, size)
| + return libnbdmod.aio_buffer_is_zero(self._o, offset, size,
| + hasattr(self, '_init'))
|
|
| class NBD(object):
---
generator/Python.ml | 20 ++--
python/handle.c | 242 +++++++++-----------------------------------
2 files changed, 56 insertions(+), 206 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index c49af4f..86781ac 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -73,9 +73,6 @@ let
) ([ "create"; "close";
"display_version";
"alloc_aio_buffer";
- "aio_buffer_from_bytearray";
- "aio_buffer_to_bytearray";
- "aio_buffer_size";
"aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n";
@@ -105,9 +102,6 @@ let
) ([ "create"; "close";
"display_version";
"alloc_aio_buffer";
- "aio_buffer_from_bytearray";
- "aio_buffer_to_bytearray";
- "aio_buffer_size";
"aio_buffer_is_zero"] @ List.map fst handle_calls);
pr " { NULL, NULL, 0, NULL }\n";
pr "};\n";
@@ -748,18 +742,21 @@ let
bytearray constructor. Otherwise, ba is copied. Either way, the
resulting AIO buffer is independent from the original.
'''
- o = libnbdmod.aio_buffer_from_bytearray(ba)
self = cls(0)
- self._o = o
+ self._o = bytearray(ba)
+ self._init = True
return self
def to_bytearray(self):
'''Copy an AIO buffer into a bytearray.'''
- return libnbdmod.aio_buffer_to_bytearray(self)
+ if not hasattr(self, '_init'):
+ self._o = bytearray(len(self._o))
+ self._init = True
+ return bytearray(self._o)
def size(self):
'''Return the size of an AIO buffer.'''
- return libnbdmod.aio_buffer_size(self)
+ return len(self._o)
def is_zero(self, offset=0, size=-1):
'''Returns true if and only if all bytes in the buffer are zeroes.
@@ -775,7 +772,8 @@ let
always returns true. If size > 0, we check the interval
[offset..offset+size-1].
'''
- return libnbdmod.aio_buffer_is_zero(self, offset, size)
+ return libnbdmod.aio_buffer_is_zero(self._o, offset, size,
+ hasattr(self, '_init'))
class NBD(object):
diff --git a/python/handle.c b/python/handle.c
index 79b369d..ca6c8e9 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -40,12 +40,6 @@
#include "methods.h"
-struct py_aio_buffer {
- Py_ssize_t len;
- void *data;
- bool initialized;
-};
-
static inline PyObject *
put_handle (struct nbd_handle *h)
{
@@ -102,242 +96,100 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args)
return Py_None;
}
-static const char aio_buffer_name[] = "nbd.Buffer";
-
-/* Return internal buffer pointer of nbd.Buffer */
-static struct py_aio_buffer *
-nbd_internal_py_get_aio_buffer (PyObject *object)
+/* Return new reference to MemoryView wrapping aio_buffer contents */
+PyObject *
+nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
{
+ PyObject *buffer = NULL;
+
if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) {
- PyObject *capsule = PyObject_GetAttrString(object, "_o");
+ buffer = PyObject_GetAttrString (object, "_o");
- return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ if (require_init && ! PyObject_HasAttrString (object, "_init")) {
+ assert (PyByteArray_Check (buffer));
+ memset (PyByteArray_AS_STRING (buffer), 0,
+ PyByteArray_GET_SIZE (buffer));
+ if (PyObject_SetAttrString (object, "_init", Py_True) < 0)
+ return NULL;
+ }
}
+ if (buffer)
+ return PyMemoryView_FromObject (buffer);
+
PyErr_SetString (PyExc_TypeError,
"aio_buffer: expecting nbd.Buffer instance");
return NULL;
}
-/* Return new reference to MemoryView wrapping aio_buffer contents */
-PyObject *
-nbd_internal_py_get_aio_view (PyObject *object, bool require_init)
-{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
-
- if (!buf)
- return NULL;
-
- if (require_init && !buf->initialized) {
- memset (buf->data, 0, buf->len);
- buf->initialized = true;
- }
-
- return PyMemoryView_FromMemory (buf->data, buf->len,
- require_init ? PyBUF_READ : PyBUF_WRITE);
-}
-
int
nbd_internal_py_init_aio_buffer (PyObject *object)
{
- struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
-
- assert (buf);
- buf->initialized = true;
+ if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ()))
+ return PyObject_SetAttrString (object, "_init", Py_True);
return 0;
}
-static void
-free_aio_buffer (PyObject *capsule)
-{
- struct py_aio_buffer *buf = PyCapsule_GetPointer (capsule, aio_buffer_name);
-
- if (buf)
- free (buf->data);
- free (buf);
-}
-
-/* Allocate a persistent buffer used for nbd_aio_pread. */
+/* Allocate an uninitialized persistent buffer used for nbd_aio_pread. */
PyObject *
nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args)
{
- struct py_aio_buffer *buf;
- PyObject *ret;
+ Py_ssize_t len;
- buf = malloc (sizeof *buf);
- if (buf == NULL) {
- PyErr_NoMemory ();
- return NULL;
- }
-
- buf->initialized = false;
if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer",
- &buf->len)) {
- free (buf);
+ &len))
return NULL;
- }
- if (buf->len < 0) {
- PyErr_SetString (PyExc_RuntimeError, "length < 0");
- free (buf);
- return NULL;
- }
- buf->data = malloc (buf->len);
- if (buf->data == NULL) {
- PyErr_NoMemory ();
- free (buf);
- return NULL;
- }
-
- ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer);
- if (ret == NULL) {
- free (buf->data);
- free (buf);
- return NULL;
- }
-
- return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args)
-{
- PyObject *obj;
- PyObject *arr = NULL;
- Py_ssize_t len;
- void *data;
- struct py_aio_buffer *buf;
- PyObject *ret;
-
- if (!PyArg_ParseTuple (args,
- "O:nbd_internal_py_aio_buffer_from_bytearray",
- &obj))
- return NULL;
-
- if (! PyByteArray_Check (obj)) {
- arr = PyByteArray_FromObject (obj);
- if (arr == NULL)
- return NULL;
- obj = arr;
- }
- data = PyByteArray_AsString (obj);
- if (!data) {
- PyErr_SetString (PyExc_RuntimeError,
- "parameter is not a bytearray or buffer");
- Py_XDECREF (arr);
- return NULL;
- }
- len = PyByteArray_Size (obj);
-
- buf = malloc (sizeof *buf);
- if (buf == NULL) {
- PyErr_NoMemory ();
- Py_XDECREF (arr);
- return NULL;
- }
-
- buf->len = len;
- buf->data = malloc (len);
- if (buf->data == NULL) {
- PyErr_NoMemory ();
- free (buf);
- Py_XDECREF (arr);
- return NULL;
- }
- memcpy (buf->data, data, len);
- Py_XDECREF (arr);
- buf->initialized = true;
-
- ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer);
- if (ret == NULL) {
- free (buf->data);
- free (buf);
- return NULL;
- }
-
- return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args)
-{
- PyObject *obj;
- PyObject *view;
- PyObject *ret;
-
- if (!PyArg_ParseTuple (args,
- "O:nbd_internal_py_aio_buffer_to_bytearray",
- &obj))
- return NULL;
-
- view = nbd_internal_py_get_aio_view (obj, true);
- if (view == NULL)
- return NULL;
-
- ret = PyByteArray_FromObject (view);
- Py_DECREF (view);
- return ret;
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args)
-{
- PyObject *obj;
- struct py_aio_buffer *buf;
-
- if (!PyArg_ParseTuple (args,
- "O:nbd_internal_py_aio_buffer_size",
- &obj))
- return NULL;
-
- buf = nbd_internal_py_get_aio_buffer (obj);
- if (buf == NULL)
- return NULL;
-
- return PyLong_FromSsize_t (buf->len);
+ /* Constructing bytearray(len) in python zeroes the memory; doing it this
+ * way gives uninitialized memory. This correctly flags negative len.
+ */
+ return PyByteArray_FromStringAndSize (NULL, len);
}
PyObject *
nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args)
{
- PyObject *obj;
- struct py_aio_buffer *buf;
+ Py_buffer buf;
Py_ssize_t offset, size;
+ int init;
+ PyObject *ret = NULL;
if (!PyArg_ParseTuple (args,
- "Onn:nbd_internal_py_aio_buffer_is_zero",
- &obj, &offset, &size))
+ "y*nnp:nbd_internal_py_aio_buffer_is_zero",
+ &buf, &offset, &size, &init))
return NULL;
- if (size == 0)
- Py_RETURN_TRUE;
-
- buf = nbd_internal_py_get_aio_buffer (obj);
- if (buf == NULL)
- return NULL;
+ if (size == 0) {
+ ret = Py_True;
+ goto out;
+ }
/* Check the bounds of the offset. */
- if (offset < 0 || offset > buf->len) {
+ if (offset < 0 || offset > buf.len) {
PyErr_SetString (PyExc_IndexError, "offset out of range");
- return NULL;
+ goto out;
}
/* Compute or check the length. */
if (size == -1)
- size = buf->len - offset;
+ size = buf.len - offset;
else if (size < 0) {
PyErr_SetString (PyExc_IndexError,
"size cannot be negative, "
"except -1 to mean to the end of the buffer");
- return NULL;
+ goto out;
}
- else if ((size_t) offset + size > buf->len) {
+ else if ((size_t) offset + size > buf.len) {
PyErr_SetString (PyExc_IndexError, "size out of range");
- return NULL;
+ goto out;
}
- if (!buf->initialized || is_zero (buf->data + offset, size))
- Py_RETURN_TRUE;
+ if (!init || is_zero (buf.buf + offset, size))
+ ret = Py_True;
else
- Py_RETURN_FALSE;
+ ret = Py_False;
+ out:
+ PyBuffer_Release (&buf);
+ Py_XINCREF (ret);
+ return ret;
}
Looks like a nice simplification!
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
11:37 a.m.
New subject: [libnbd PATCH v2 8/8] python: Make nbd.Buffer lighter-weight
On Tue, Jun 07, 2022 at 02:20:35PM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:33PM -0500, Eric Blake wrote:
> Instead of storing a PyCapsule in _o and repeatedly doing lookups to
> dereference a stored malloc'd pointer, it is easier to just directly
> store a Python buffer-like object as _o. Track initialization via
> ._init: absent for nbd.Buffer(int), present for
> nbd.Buffer.from_bytearray or after we have forced initialization due
> to aio_pread or .to_bytearray. At this point, we are still copying in
> and out of .{from,to}_bytearray, but we are getting closer to reducing
> the number of copies. The sheer size in reduced lines of code is a
> testament to the ease of doing things closer to Python, rather than
> hidden behind unpacking a PyCapsule layer.
>
Looks like a nice simplification!
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Series now in as 08a3804..4f15d0e. Next up - fixing
[aio_]pread_structured to expose slices of the underlying Python
object, and teaching nbd.Buffer to reuse (rather than copy) buffers.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
901
days inactive
901
days old
22 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones