[Libguestfs] [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