Take advantage of the previous patch to make it easier for language bindings to affect the exact error they want on failure, rather than requiring them to influence errno. Update the python test to tickle the changed bindings, and to prove that we can at least trigger an exception, although we are still lacking bindings for python code to access the last NBD exception and error code gracefully (maybe we should be returning a specific subclass of Exception that contains the last error information). --- Applies on top of my fixes to Rich's proposal for a Mutable(Int) callback parameter in the generator. I validated that: except Exception as e: print(e) in 405-pread-structured.py is sufficient to produce output resembling: nbd_pread_structured: read: command failed: Protocol error Traceback (most recent call last): File "./t/405-pread-structured.py", line 48, in <module> buf = h.pread_structured (512, 0, 43, f, nbd.CMD_FLAG_DF) File "/home/eblake/libnbd/python/nbd.py", line 517, in pread_structured return libnbdmod.pread_structured (self._o, count, offset, data, chunk, flag s) RuntimeError: nbd_pread_structured: read: command failed: Protocol error where assigning to error.value in the callback affected the strerror reference in the RuntimeError; but we really should be thinking about returning a subclass of RuntimeError for programmatic access from python rather than just scraping the generic error string. generator/generator | 28 ++++++++++++++++------------ generator/states-reply-simple.c | 7 ++++--- generator/states-reply-structured.c | 21 ++++++++++++--------- interop/structured-read.c | 6 +++--- lib/internal.h | 2 +- python/t/405-pread-structured.py | 12 +++++++++++- tests/oldstyle.c | 8 ++++---- 7 files changed, 51 insertions(+), 33 deletions(-) diff --git a/generator/generator b/generator/generator index a9a23f4..37fa0fc 100755 --- a/generator/generator +++ b/generator/generator @@ -1330,7 +1330,7 @@ protocol extensions)."; args = [ BytesOut ("buf", "count"); UInt64 "offset"; Opaque "data"; Callback ("chunk", [ Opaque "data"; BytesIn ("subbuf", "count"); - UInt64 "offset"; Int "error"; + UInt64 "offset"; Mutable (Int "error"); Int "status" ]); Flags "flags" ]; ret = RErr; @@ -1346,8 +1346,9 @@ additional sanity checking on the server's reply. The callback cannot call C<nbd_*> APIs on the same handle since it holds the handle lock and will cause a deadlock. If the callback returns C<-1>, and no earlier error has been detected, then the overall read command will -fail with the same value of C<errno> left after the failing callback; -but any further chunks will still invoke the callback. +fail with any non-zero value stored into the callback's C<error> +parameter (with a default of C<EPROTO>); but any further chunks will +still invoke the callback. The C<chunk> function is called once per chunk of data received. The C<subbuf> and C<count> parameters represent the subset of the original @@ -1356,25 +1357,27 @@ C<subbuf> always points within the original C<buf>; but this guarantee may not extend to other language bindings). The C<offset> parameter represents the absolute offset at which C<subbuf> begins within the image (note that this is not the relative offset of C<subbuf> within -the original buffer C<buf>). The C<error> parameter is controlled by -the C<status> parameter, which is one of +the original buffer C<buf>). Changes to C<error> on output are ignored +unless the callback fails. The input meaning of the C<error> parameter +is controlled by the C<status> parameter, which is one of =over 4 =item C<LIBNBD_READ_DATA> = 1 -C<subbuf> was populated with C<count> bytes of data. C<error> is the -errno value of any earlier detected error, or zero. +C<subbuf> was populated with C<count> bytes of data. On input, C<error> +contains the errno value of any earlier detected error, or zero. =item C<LIBNBD_READ_HOLE> = 2 -C<subbuf> represents a hole, and contains C<count> NUL bytes. C<error> -is the errno value of any earlier detected error, or zero. +C<subbuf> represents a hole, and contains C<count> NUL bytes. On input, +C<error> contains the errno value of any earlier detected error, or zero. =item C<LIBNBD_READ_ERROR> = 3 -C<count> is 0, so C<subbuf> is unusable. C<error> is the errno value -reported by the server as occurring while reading that C<offset>. +C<count> is 0, so C<subbuf> is unusable. On input, C<error> contains the +errno value reported by the server as occurring while reading that +C<offset>, regardless if any earlier error has been detected. =back @@ -1685,7 +1688,8 @@ parameters behave as documented in C<nbd_pread>."; Opaque "data"; CallbackPersist ("chunk", [ Opaque "data"; BytesIn ("subbuf", "count"); - UInt64 "offset"; Int "error"; + UInt64 "offset"; + Mutable (Int "error"); Int "status" ]); Flags "flags" ]; ret = RInt64; diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 25eab9d..cab72d6 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -61,11 +61,12 @@ /* guaranteed by START */ assert (cmd); if (cmd->cb.fn.read) { + int error = 0; + assert (cmd->error == 0); - errno = 0; if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count, - cmd->offset, 0, LIBNBD_READ_DATA) == -1) - cmd->error = errno ? errno : EPROTO; + cmd->offset, &error, LIBNBD_READ_DATA) == -1) + cmd->error = error ? error : EPROTO; } SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 24bead6..fa11dd6 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -292,15 +292,16 @@ return -1; } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) { + int scratch = error; + /* Different from successful reads: inform the callback about the * current error rather than any earlier one. If the callback fails * without setting errno, then use the server's error below. */ - errno = 0; if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset - cmd->offset), - 0, offset, error, LIBNBD_READ_ERROR) == -1) + 0, offset, &scratch, LIBNBD_READ_ERROR) == -1) if (cmd->error == 0) - cmd->error = errno; + cmd->error = scratch; } } @@ -381,12 +382,13 @@ assert (cmd); /* guaranteed by CHECK */ if (cmd->cb.fn.read) { - errno = 0; + int error = cmd->error; + if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset - cmd->offset), - length - sizeof offset, offset, cmd->error, + length - sizeof offset, offset, &error, LIBNBD_READ_DATA) == -1) if (cmd->error == 0) - cmd->error = errno ? errno : EPROTO; + cmd->error = error ? error : EPROTO; } SET_NEXT_STATE (%FINISH); @@ -441,12 +443,13 @@ */ memset (cmd->data + offset, 0, length); if (cmd->cb.fn.read) { - errno = 0; + int error = cmd->error; + if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + offset, length, - cmd->offset + offset, cmd->error, + cmd->offset + offset, &error, LIBNBD_READ_HOLE) == -1) if (cmd->error == 0) - cmd->error = errno ? errno : EPROTO; + cmd->error = error ? error : EPROTO; } SET_NEXT_STATE(%FINISH); diff --git a/interop/structured-read.c b/interop/structured-read.c index cf8b893..46a7a80 100644 --- a/interop/structured-read.c +++ b/interop/structured-read.c @@ -47,7 +47,7 @@ struct data { static int read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset, - int error, int status) + int *error, int status) { struct data *data = opaque; const char *buf = bufv; @@ -55,7 +55,7 @@ read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset, /* The NBD spec allows chunks to be reordered; we are relying on the * fact that qemu-nbd does not do so. */ - assert (!error || (data->fail && data->count == 1)); + assert (!*error || (data->fail && data->count == 1 && *error == EPROTO)); assert (data->count-- > 0); switch (status) { @@ -93,7 +93,7 @@ read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset, if (data->fail) { /* Something NBD servers can't send */ - errno = data->count == 1 ? EPROTO : ECONNREFUSED; + *error = data->count == 1 ? EPROTO : ECONNREFUSED; return -1; } return 0; diff --git a/lib/internal.h b/lib/internal.h index 7d87fa4..f827957 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -234,7 +234,7 @@ struct socket { typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries); typedef int (*read_fn) (void *data, const void *buf, size_t count, - uint64_t offset, int error, int status); + uint64_t offset, int *error, int status); struct command_cb { void *opaque; diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py index 1bfa162..9ed2090 100644 --- a/python/t/405-pread-structured.py +++ b/python/t/405-pread-structured.py @@ -16,6 +16,7 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import nbd +import errno h = nbd.NBD () h.connect_command (["nbdkit", "-s", "--exit-with-parent", "-v", @@ -24,10 +25,11 @@ h.connect_command (["nbdkit", "-s", "--exit-with-parent", "-v", expected = b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00(\x00\x00\x00\x00\x00\x00\x000\x00\x00\x00\x00\x00\x00\x008\x00\x00\x00\x00\x00\x00\x00@\x00\x00\x00\x00\x00\x00\x00H\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00`\x00\x00\x00\x00\x00\x00\x00h\x00\x00\x00\x00\x00\x00\x00p\x00\x00\x00\x00\x00\x00\x00x\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x90\x00\x00\x00\x00\x00\x00\x00\x98\x00\x00\x00\x00\x00\x00\x00\xa0\x00\x00\x00\x00\x00\x00\x00\xa8\x00\x00\x00\x00\x00\x00\x00\xb0\x00\x00\x00\x00\x00\x00\x00\xb8\x00\x00\x00\x00\x00\x00\x00\xc0\x00\x00\x00\x00\x00\x00\x00\xc8\x00\x00\x00\x00\x00\x00\x00\xd0\x00\x00\x00\x00\x00\x00\x00\xd8\x00\x00\x00\x00\x00\x00\x00\xe0\x00\x00\x00\x00\x00\x00\x00\xe8\x00\x00\x00\x00\x00\x00\x00\xf0\x00\x00\x00\x00\x00\x00\x00\xf8\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x01\x08\x00\x00\x00\x00\x00\x00\x01\x10\x00\x00\x00\x00\x00\x00\x01\x18\x00\x00\x00\x00\x00\x00\x01 \x00\x00\x00\x00\x00\x00\x01(\x00\x00\x00\x00\x00\x00\x010\x00\x00\x00\x00\x00\x00\x018\x00\x00\x00\x00\x00\x00\x01@\x00\x00\x00\x00\x00\x00\x01H\x00\x00\x00\x00\x00\x00\x01P\x00\x00\x00\x00\x00\x00\x01X\x00\x00\x00\x00\x00\x00\x01`\x00\x00\x00\x00\x00\x00\x01h\x00\x00\x00\x00\x00\x00\x01p\x00\x00\x00\x00\x00\x00\x01x\x00\x00\x00\x00\x00\x00\x01\x80\x00\x00\x00\x00\x00\x00\x01\x88\x00\x00\x00\x00\x00\x00\x01\x90\x00\x00\x00\x00\x00\x00\x01\x98\x00\x00\x00\x00\x00\x00\x01\xa0\x00\x00\x00\x00\x00\x00\x01\xa8\x00\x00\x00\x00\x00\x00\x01\xb0\x00\x00\x00\x00\x00\x00\x01\xb8\x00\x00\x00\x00\x00\x00\x01\xc0\x00\x00\x00\x00\x00\x00\x01\xc8\x00\x00\x00\x00\x00\x00\x01\xd0\x00\x00\x00\x00\x00\x00\x01\xd8\x00\x00\x00\x00\x00\x00\x01\xe0\x00\x00\x00\x00\x00\x00\x01\xe8\x00\x00\x00\x00\x00\x00\x01\xf0\x00\x00\x00\x00\x00\x00\x01\xf8' def f (data, buf2, offset, err, s): + assert err.value == 0 + err.value = errno.EPROTO assert data == 42 assert buf2 == expected assert offset == 0 - assert err == 0 assert s == nbd.READ_DATA buf = h.pread_structured (512, 0, 42, f) @@ -41,3 +43,11 @@ buf = h.pread_structured (512, 0, 42, f, nbd.CMD_FLAG_DF) print ("%r" % buf) assert buf == expected + +try: + buf = h.pread_structured (512, 0, 43, f, nbd.CMD_FLAG_DF) + assert False +except Exception: + # Need a way for python to access last NBD error... + # assert nbd.get_errno == errno.EPROTO + pass diff --git a/tests/oldstyle.c b/tests/oldstyle.c index 38f5130..0207bf8 100644 --- a/tests/oldstyle.c +++ b/tests/oldstyle.c @@ -36,7 +36,7 @@ static const char *progname; static int pread_cb (void *data, const void *buf, size_t count, uint64_t offset, - int error, int status) + int *error, int status) { int *calls = data; ++*calls; @@ -49,7 +49,7 @@ pread_cb (void *data, const void *buf, size_t count, uint64_t offset, fprintf (stderr, "%s: callback called with wrong offset\n", progname); exit (EXIT_FAILURE); } - if (error != 0) { + if (*error != 0) { fprintf (stderr, "%s: callback called with unexpected error\n", progname); exit (EXIT_FAILURE); } @@ -64,7 +64,7 @@ pread_cb (void *data, const void *buf, size_t count, uint64_t offset, } if (*calls > 1) { - errno = EPROTO; /* Something NBD servers can't send */ + *error = ECONNREFUSED; /* Something NBD servers can't send */ return -1; } @@ -143,7 +143,7 @@ main (int argc, char *argv[]) fprintf (stderr, "%s: expected failure from callback\n", argv[0]); exit (EXIT_FAILURE); } - if (nbd_get_errno () != EPROTO) { + if (nbd_get_errno () != ECONNREFUSED) { fprintf (stderr, "%s: wrong errno value after failed callback\n", argv[0]); exit (EXIT_FAILURE); } -- 2.20.1