search for: libnbd_read_error

...rno_of_nbd_error (error); > + set_error (errno, "server reported read failure at offset 0x%" PRIx64, > + offset); > + if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset - cmd->offset), > + 0, offset, LIBNBD_READ_ERROR) == -1) I understand that in the next commit the pread_callback callback function is meant to read errno implicitly, and this is why you are setting it before calling that callback here. However I don't understand why you didn't just pass this in as an extra (ie. explicit) parameter? As...

...of wiring things up to the various language bindings. It's easy to test callback behavior for LIBNBD_READ_DATA/HOLE (the next patch will add interop tests with qemu-nbd), but at present, I don't know of any server that responds with NBD_REPLY_TYPE_ERROR_OFFSET, which in turn makes testing LIBNBD_READ_ERROR difficult. I used the following hack on top of qemu.git commit d1bf88e5 to trigger an offset error for local testing: | diff --git i/nbd/server.c w/nbd/server.c | index 10faedcfc55d..804dced0bc8d 100644 | --- i/nbd/server.c | +++ w/nbd/server.c | @@ -1838,12 +1838,30 @@ static int coroutine_fn nbd...

...fails * without setting errno, then use the server's error below. */ - if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, - cmd->data + (offset - cmd->offset), - 0, offset, LIBNBD_READ_ERROR, - &scratch) == -1) + if (CALL_CALLBACK (cmd->cb.fn.chunk, + cmd->data + (offset - cmd->offset), + 0, offset, LIBNBD_READ_ERROR, + &scratch) == -1)...

...;offset); + assert (offset + count <= data->offset + data->count); + + switch (status) { + case LIBNBD_READ_DATA: + total_data_chunks++; + total_data_bytes += count; + break; + case LIBNBD_READ_HOLE: + total_hole_chunks++; + total_hole_bytes += count; + break; + case LIBNBD_READ_ERROR: + assert (count == 0); + count = 1; /* Ensure no further chunks visit that offset */ + break; + default: + goto error; + } + data->chunks++; + if (count == 0) { + fprintf (stderr, "buggy server: chunk must have non-zero size\n"); + goto error; + } + + /* Find e...

...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 a...

...to the generator to support an Int type in a callback. It's easy to test callback behavior for LIBNBD_READ_DATA/HOLE (the next patch will add interop tests with qemu-nbd), but at present, I don't know of any server that responds with NBD_REPLY_TYPE_ERROR_OFFSET, which in turn makes testing LIBNBD_READ_ERROR difficult. I used the following hack on top of qemu.git commit d1bf88e5 to trigger an offset error for local testing: | diff --git i/nbd/server.c w/nbd/server.c | index 10faedcfc55d..804dced0bc8d 100644 | --- i/nbd/server.c | +++ w/nbd/server.c | @@ -1838,12 +1838,30 @@ static int coroutine_fn nbd...

...*/ >> - if (cmd->cb.fn.read (cmd->cb.fn_user_data, >> + if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data, >> cmd->data + (offset - cmd->offset), >> 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) >> if (cmd->error == 0) > > We could still optimize this file based on NBD_REPLY_FLAG_DONE, but that > can be a followup. > >> @@ -499,7 +499,7 @@ >> /* Call the caller's extent function. */ >> int error = c...

...or below. > */ > - if (cmd->cb.fn.read (cmd->cb.fn_user_data, > + if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data, > cmd->data + (offset - cmd->offset), > 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) > if (cmd->error == 0) We could still optimize this file based on NBD_REPLY_FLAG_DONE, but that can be a followup. > @@ -499,7 +499,7 @@ > /* Call the caller's extent function. */ > int error = cmd->error; > > - if...

...e server's error below. */ - if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data, + if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data, cmd->data + (offset - cmd->offset), 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; + if (valid & LIBNBD_CALLBACK_FREE) + cmd->cb.fn.read = NULL; /* because we've freed it */ } } @@ -384,13 +400,16 @@ assert (cmd); /* guaranteed by CHECK */...

Since v1: - rebase to applied patches - split out support for Int in callbacks - sort of test that callbacks work in OCaml (see comment in patch 5) - rename API to nbd_pread_structured - expose error as explicit parameter to callback Eric Blake (5): generator: Allow Int in callbacks states: Wire in a read callback states: Add nbd_pread_structured API states: Add tests for

...no = nbd_internal_errno_of_nbd_error (error); + set_error (errno, "server reported read failure at offset 0x%" PRIx64, + offset); + if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset - cmd->offset), + 0, offset, LIBNBD_READ_ERROR) == -1) + if (cmd->error == 0 && errno) + cmd->error = errno; + } } /* Preserve first error encountered */ @@ -304,9 +318,27 @@ return 0; REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA: + struct command_in_flight *cmd = h->reply_cmd; + uint...

...diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index cdd9f10..b016cd7 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -317,8 +317,10 @@ valid_flags (struct nbd_handle *h) 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; - if (valid & LIBNBD_CALLBACK_FREE) + if (valid & LIBNBD_CALLBACK_FREE) { cmd->cb.fn.chunk = NULL; /* because we've freed it */ + nbd_internal_free_callbac...

...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) + if (cmd->error == 0) + cmd->error = errno; + } } /* Preserve first error encountered */ @@ -313,9 +327,27 @@ return 0; REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA: + struct command_in_flight *cmd = h->reply_cmd; + uint64_t offset; + u...

...earlier one. If the callback fails * without setting errno, then use the server's error below. */ - if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data, - cmd->data + (offset - cmd->offset), - 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) + if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data, + cmd->data + (offset - cmd->offset), + 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) if (cmd->error == 0) cmd->...

...if (cmd->cb.fn.chunk.callback (valid, cmd->cb.fn.chunk.user_data, > + if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, > cmd->data + (offset - cmd->offset), > 0, offset, LIBNBD_READ_ERROR, > &scratch) == -1) > if (cmd->error == 0) > cmd->error = scratch; > - if (valid & LIBNBD_CALLBACK_FREE) > + if (flags & NBD_REPLY_FLAG_DONE) { > + if (cmd->cb.fn.chunk.free...

Patches 1 & 2 are rather complex, but the end result is that we pass closures + user_data + free function in single struct parameters as I described previously in this email: https://www.redhat.com/archives/libguestfs/2019-August/msg00210.html Patch 3 adds a convenient FREE_CALLBACK macro which seems a worthwhile simplification if you buy into 1 & 2. Patch 4 adds another macro which is

...lier one. If the callback fails * without setting errno, then use the server's error below. */ - if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data, - cmd->data + (offset - cmd->offset), - 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) + if (cmd->cb.fn.chunk.callback (valid, cmd->cb.fn.chunk.user_data, + cmd->data + (offset - cmd->offset), + 0, offset, LIBNBD_READ_ERROR, + &amp...

I've mentioned this topic before (in fact, the idea of adding NBD_CMD_FLAG_DF was first mentioned at [1]), but finally finished enough of an implementation to feel confident in posting it. I'd still like to add something under examples/ that uses the new API to implement strict checking of a server's structured replies read implementation (ensure that a server never sends data after

** INCOMPLETE ** This is the generator change as discussed on the list already. The Python and OCaml bindings are not yet done. It passes all [C only] tests and valgrind. Note that nbd_add_close_callback is inconsistent with other closure types because it passes the user_data parameter after the function. (This is not caused by the current patch, it was already inconsistent). We decided that

I think I've addressed everything that was raised in review. Some of the highlights: - Callbacks should be freed reliably along all exit paths. - There's a simple test of closure lifetimes. - I've tried to use VALID|FREE in all the places where I'm confident that it's safe and correct to do. There may be more places. Note this is an optimization and shouldn't