search for: libnbd_read_data

...ator/states-reply-simple.c @@ -64,10 +64,10 @@ int error = 0; assert (cmd->error == 0); - if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, - cmd->data, cmd->count, - cmd->offset, LIBNBD_READ_DATA, - &error) == -1) + if (CALL_CALLBACK (cmd->cb.fn.chunk, + cmd->data, cmd->count, + cmd->offset, LIBNBD_READ_DATA, + &error) == -1) cmd->error = error ? e...

Time to wire up user access to react to structured reads as the chunks come in, exposing the framework added in the last patch. I chose to use Int for the callback status rather than an enum for ease 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...

..._internal_retire_and_free_command (struct command *); diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 8e3d7f1..2f83e6f 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -69,7 +69,6 @@ cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; - FREE_CALLBACK (cmd->cb.fn.chunk); } SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 2e327ce..58e83d4 10...

...y -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, o...

...NEXT_STATE (%^FINISH_COMMAND); + case 0: + /* guaranteed by START */ + assert (cmd); + if (cmd->cb.fn.read) { + assert (cmd->error == 0); + errno = 0; + if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count, + cmd->offset, LIBNBD_READ_DATA) == -1) + cmd->error = errno ? errno : EPROTO; + } + + SET_NEXT_STATE (%^FINISH_COMMAND); } return 0; diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 657106e..00659de 100644 --- a/generator/states-reply-structured.c +++ b/generator...

...-git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 9b249ab..9684bc4 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -69,6 +69,7 @@ cmd->data, cmd->count, cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; + nbd_internal_free_callback (h, cmd->cb.fn_user_data); cmd->cb.fn.chunk = NULL; /* because we've freed it */ } diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c in...

...T_STATE (%^FINISH_COMMAND); + case 0: + /* guaranteed by START */ + assert (cmd); + if (cmd->cb.fn.read) { + 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; + } + + SET_NEXT_STATE (%^FINISH_COMMAND); } return 0; diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 9dcc898..f1d421d 100644 --- a/generator/states-reply-structured.c +++ b/generator...

...hanged, 35 insertions(+), 56 deletions(-) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 7f2775d..8905367 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -69,9 +69,7 @@ cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; - if (cmd->cb.fn.chunk.free) - cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); - cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */ + FREE...

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

...struct command *); > > diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c > index 8e3d7f1..2f83e6f 100644 > --- a/generator/states-reply-simple.c > +++ b/generator/states-reply-simple.c > @@ -69,7 +69,6 @@ > cmd->offset, LIBNBD_READ_DATA, > &error) == -1) > cmd->error = error ? error : EPROTO; > - FREE_CALLBACK (cmd->cb.fn.chunk); > } > > SET_NEXT_STATE (%^FINISH_COMMAND); > diff --git a/generator/states-reply-structured.c b/generator/states-reply-str...

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

...ework added in the last patch. I chose to use an int for the callback status rather than an enum for ease of wiring things up to the various language bindings. Still, it required quite a few tweaks 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...

...t;error == 0); > - if (cmd->cb.fn.read (cmd->cb.fn_user_data, cmd->data, cmd->count, > + if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data, > + cmd->data, cmd->count, > cmd->offset, LIBNBD_READ_DATA, &error) == -1) [2] Here, we know we are calling the read callback exactly once, so we could pass DATA|FREE here. > cmd->error = error ? error : EPROTO; > } > diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c > index f60232e..2...

...n.chunk) { int error = 0; assert (cmd->error == 0); - if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.fn_user_data, - cmd->data, cmd->count, - cmd->offset, LIBNBD_READ_DATA, &error) == -1) + if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, + cmd->cb.fn_user_data, + cmd->data, cmd->count, + cmd->offset, LIBNBD_READ_DATA, &error) == -1)...

...- 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), length - sizeof offset, offset, LIBNBD_READ_DATA, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; + if (valid & LIBNBD_CALLBACK_FREE) + cmd->cb.fn.read = NULL; /* because we've freed it */ } SET_NEXT_STATE (%FINISH); @@ -446,13 +465,16 @@ memset (cmd-&gt...

...> - if (cmd->cb.fn.read (cmd->cb.fn_user_data, cmd->data, cmd->count, > > + if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data, > > + cmd->data, cmd->count, > > cmd->offset, LIBNBD_READ_DATA, &error) == -1) > > [2] Here, we know we are calling the read callback exactly once, so we > could pass DATA|FREE here. That was actually my first version of the patch. However it runs into the problem that we need to call the FREE version when a command is aborted. It's compli...

...ack) { int error = 0; assert (cmd->error == 0); - if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.fn_user_data, - cmd->data, cmd->count, - cmd->offset, LIBNBD_READ_DATA, &error) == -1) + if (cmd->cb.fn.chunk.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, + cmd->cb.fn.chunk.user_data, + cmd->data, cmd->count, + cmd->offset, LIBN...

...nt count; /* input: count of expected remaining calls */ bool fail; /* input: true to return failure */ bool seen_hole; /* output: true if hole encountered */ @@ -60,15 +60,24 @@ read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset, switch (status) { case LIBNBD_READ_DATA: - // XXX if (df...) - assert (buf == rbuf + 512); - assert (count == 512); - assert (offset == 2048 + 512); - assert (buf[0] == 1 && memcmp (buf, buf + 1, 511) == 0); + if (data->df) { + assert (buf == rbuf); + assert (count == 1024); + assert (offset ==...

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

...t count, uint64_t offset, + int *error, int status) +{ + struct data *data = opaque; + struct range *r, **prev; + + /* libnbd guarantees this: */ + assert (offset >= data->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; +...