[Libguestfs] [libnbd PATCH 2/5] python: Don't unwrap nbd.Buffer in nbd.py
Richard W.M. Jones
rjones at redhat.com
Sat Jun 4 10:08:25 UTC 2022
On Fri, Jun 03, 2022 at 05:26:32PM -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-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
(Checks patch #3 ... yes ... this code does get simplified further :-)
> 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;
> +}
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
--
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
More information about the Libguestfs
mailing list