Re: [Libguestfs] [libnbd PATCH 2/5] 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-03 12:16:58.200633552 -0500 | +++ python/methods.h 2022-06-03 12:17:16.605661419 -0500 | @@ -38,6 +38,7 @@ | 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_nbd_buffer_type (void); | | static inline struct nbd_handle * | get_handle (PyObject *obj) | --- python/methods.c.bak 2022-06-02 14:54:37.016940545 -0500 | +++ python/methods.c 2022-06-03 12:17:16.615661434 -0500 | @@ -3162,8 +3162,8 @@ 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 */ | - struct py_aio_buffer *buf_buf; | + PyObject *buf; /* instance of nbd.Buffer */ | + struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *completion_user_data = NULL; | @@ -3221,8 +3221,8 @@ 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 */ | - struct py_aio_buffer *buf_buf; | + PyObject *buf; /* instance of nbd.Buffer */ | + struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *chunk_user_data = NULL; | @@ -3296,8 +3296,8 @@ 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 */ | - struct py_aio_buffer *buf_buf; | + PyObject *buf; /* instance of nbd.Buffer */ | + struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *completion_user_data = NULL; | --- python/nbd.py.bak 2022-06-03 12:16:28.320588309 -0500 | +++ python/nbd.py 2022-06-03 12:38:01.974645685 -0500 | @@ -136,11 +136,11 @@ 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''' | - return libnbdmod.aio_buffer_size(self._o) | + return libnbdmod.aio_buffer_size(self) | | def is_zero(self, offset=0, size=-1): | ''' | @@ -154,7 +154,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): | @@ -2201,8 +2201,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): | @@ -2236,8 +2235,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): | '''▶ write to the NBD server | @@ -2260,8 +2259,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): | '''▶ disconnect from the NBD server --- generator/Python.ml | 20 ++++++++++---------- python/handle.c | 11 +++++++++-- python/utils.c | 20 +++++++++++++++++++- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 3f672ba..fcab6bd 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -44,6 +44,7 @@ let 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_nbd_buffer_type (void); static inline struct nbd_handle * get_handle (PyObject *obj) @@ -299,9 +300,8 @@ 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 " struct py_aio_buffer *%s_buf;\n" n + pr " PyObject *%s; /* instance of nbd.Buffer */\n" n; + pr " struct py_aio_buffer *%s_buf; /* Contents of nbd.Buffer */\n" n | Closure { cbname } -> pr " struct user_data *%s_user_data = NULL;\n" cbname; pr " PyObject *py_%s_fn;\n" cbname; @@ -357,9 +357,9 @@ let function | Bool n -> pr " \"p\"" | BytesIn (n, _) -> pr " \"y*\"" - | BytesPersistIn (n, _) -> pr " \"O\"" + | BytesPersistIn _ -> pr " \"O\"" | BytesOut (_, count) -> pr " \"n\"" - | BytesPersistOut (_, count) -> pr " \"O\"" + | BytesPersistOut _ -> pr " \"O\"" | Closure _ -> pr " \"O\"" | Enum _ -> pr " \"i\"" | Flags _ -> pr " \"I\"" @@ -776,11 +776,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): ''' @@ -794,7 +794,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): @@ -819,8 +819,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 9fe3f8e..f84c6e0 100644 --- a/python/handle.c +++ b/python/handle.c @@ -99,9 +99,16 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args) static const char aio_buffer_name[] = "nbd.Buffer"; struct py_aio_buffer * -nbd_internal_py_get_aio_buffer (PyObject *capsule) +nbd_internal_py_get_aio_buffer (PyObject *buffer) { - return PyCapsule_GetPointer (capsule, aio_buffer_name); + if (PyObject_IsInstance (buffer, nbd_internal_py_get_nbd_buffer_type ())) { + PyObject *capsule = PyObject_GetAttrString(buffer, "_o"); + return PyCapsule_GetPointer (capsule, aio_buffer_name); + } + + PyErr_SetString (PyExc_TypeError, + "aio_buffer: expecting nbd.Buffer instance"); + return NULL; } static void diff --git a/python/utils.c b/python/utils.c index 37f0c55..bc7d6a1 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"); + Py_DECREF (modname); + Py_DECREF (module); + } + assert (type); + return type; +}