search for: nbd_internal_command_common

...| 20 +++++++++++++++++++- 5 files changed, 60 insertions(+), 27 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 2a5147f..cde5dcd 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -432,7 +432,8 @@ extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type); extern int64_t nbd_internal_command_common (struct nbd_handle *h, uint16_t flags, uint16_t type, uint64_t offset, uint64_t count, - void *data, struct command_cb *cb); +...

...NBD_CMD_DISC was open-coding things. Furthermore, every caller was triggering nbd_internal_run() for the cmd_issue event; doing that in a central place makes it easier for the next patch to improve that logic without duplicating the fix at each caller. Fix these things by renaming the function to nbd_internal_command_common, which is necessary for exporting it, and by adding parameters and an updated return type. --- lib/disconnect.c | 20 +++------ lib/internal.h | 7 ++++ lib/rw.c | 103 +++++++++++++---------------------------------- 3 files changed, 39 insertions(+), 91 deletions(-) diff --git a/lib...

...the generator will now blindly free anything that has not been cleared by the unlocked_* helper, which in turn are now careful to clear any callback once it has successfully been copied into somewhere that will guarantee subsequent cleanup (whether as part of the state machine, or by the fact that nbd_internal_command_common now cleans up on all error paths). Yes, our change means that any client that WAS checking for nbd_aio_* returning -1 for cookies and manually cleaning up is now performing a double free, but as argued above, such clients are unlikely because of the extra boilerplate it would entail, and because o...

As promised in my earlier thread on libnbd completion callback question. Eric Blake (2): generator: Refactor handling of closures in unlocked functions generator: Free closures on failure docs/libnbd.pod | 2 +- generator/C.ml | 48 +++++++++++------ generator/C.mli | 1 + lib/debug.c | 7 +-- lib/opt.c | 31 ++++++-----

...NDS) { + if (count == 0) { + set_error (EINVAL, "count cannot be 0"); + return -1; + } + + if (offset > h->exportsize || offset + count > h->exportsize) { + set_error (EINVAL, "request out of bounds"); + return -1; + } + } + return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, buf, &cb); } @@ -305,6 +317,18 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, } } + if (h->strict & LIBNBD_STRICT_BOUNDS) { + if (count == 0) { + set_error (EINVAL, &quot...

...%s);\n" n n; pr " ret = %s;\n" value; diff --git a/lib/disconnect.c b/lib/disconnect.c index 9de1e34..f99fbd0 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -64,7 +64,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); + id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, NULL, NULL); if (id == -1) return -1; h->disconnect_request = true; diff --git a/lib/rw.c b/lib/rw.c index f49fe25..e3e80ad 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -281,7 +281,7...

We have been inconsistent on how much we reject client-side without even consulting the server, vs. how much we depend on the server to detect failure (even if our request can be deemed undefined per NBD protocol). I'd like to change it so that by default, we reject as much as we can client-side for less traffic, but where the user can also change things on the fly for server-side integration

...s;\n" value; > diff --git a/lib/disconnect.c b/lib/disconnect.c > index 9de1e34..f99fbd0 100644 > --- a/lib/disconnect.c > +++ b/lib/disconnect.c > @@ -64,7 +64,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) > { > int64_t id; > > - id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); > + id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, NULL, NULL); > if (id == -1) > return -1; > h->disconnect_request = true; > diff --git a/lib/rw.c b/lib/rw.c > index f49fe25..e3e80ad 100644 > --- a/lib/rw...

...extent_fn; + void *data; /* Buffer for read/write */ + struct command_cb cb; bool data_seen; /* For read, true if at least one data chunk seen */ uint32_t error; /* Local errno value */ }; @@ -300,7 +309,7 @@ extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type); extern int64_t nbd_internal_command_common (struct nbd_handle *h, uint16_t flags, uint16_t type, uint64_t offset, uint64_t count, - void *data, extent_fn extent); + v...

In v2: - now based on my proposal to add LIBNBD_SHUTDOWN_IMMEDIATE - four flags instead of two: STRICT_FLAGS is new (patch 4), and STRICT_BOUNDS is separate from STRICT_ZERO_SIZE (patch 5) - various refactorings for more shared code and less duplication Eric Blake (5): api: Add xxx_MASK constant for each Flags type generator: Refactor filtering of accepted OFlags api: Add

Since v2: - rebase to Rich's new API calls - more refactoring in patch 1 (retitled) - new patches 3 and 4 - fix data corruption in patch 6 (was 4) - more tweaks to the reproducer example (including using new API from 3) Eric Blake (7): lib: Refactor command_common() to do more common work commands: Allow for a command queue commands: Expose FIFO ordering of server completions

...t64_t id; - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (flags != 0) { + set_error (EINVAL, "invalid flag: %" PRIu32, flags); + return -1; + } } id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); diff --git a/lib/handle.c b/lib/handle.c index 7987d59..a1fa142 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -75,6 +75,8 @@ nbd_create (void) s = getenv ("LIBNBD_DEBUG"); h->debug = s && strcmp (s, "1") == 0; + h->...

...;& + (nbd_internal_is_state_ready (get_next_state (h)) || + nbd_internal_is_state_processing (get_next_state (h)))) { if (nbd_unlocked_aio_disconnect (h, 0) == -1) return -1; } @@ -57,6 +58,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); if (id == -1) return -1; + h->disconnect_request = true; /* This will leave the command on the in-flight list. Is this a * problem? Probably it isn't. If it is, we could add a flag to diff --git a/lib/internal.h b/lib/internal.h ind...

...re APIs to track the eventual completion, and will eventually learn that the state machine has moved to DEAD. --- lib/rw.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/rw.c b/lib/rw.c index 51ee691..6d37daa 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -229,6 +229,12 @@ nbd_internal_command_common (struct nbd_handle *h, /* Add the command to the end of the queue. Kick the state machine * if there is no other command being processed, otherwise, it will * be handled automatically on a future cycle around to READY. + * Beyond this point, we have to return a cookie to the user, sinc...

...;close (conn->sock); diff --git a/lib/rw.c b/lib/rw.c index 8f6227d..594593a 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -24,6 +24,7 @@ #include <stdint.h> #include <inttypes.h> #include <errno.h> +#include <assert.h> #include "internal.h" @@ -247,7 +248,15 @@ nbd_internal_command_common (struct nbd_connection *conn, uint64_t offset, uint64_t count, void *data, int64_t id, extent_fn extent) { - struct command_in_flight *cmd; + struct command_in_flight *cmd, *prev_cmd; + + if (!nbd_unlocked_aio_is_ready (conn) &&amp...

...REPLY_TYPE_ERROR*. For this specific > error case, the bookkeeping is much simpler - we merely track if we've > seen at least one data chunk. I guess the implicit assumption here is that count > 0, which IIRC the NBD protocol demands. It seems like we don't actually check this in nbd_internal_command_common so would it be worth adding a check there? In any case this patch on its own looks good, so ACK. Rich. > generator/states-reply-simple.c | 1 + > generator/states-reply-structured.c | 2 ++ > lib/aio.c | 2 ++ > lib/internal.h | 1 +...

...We could silently accept flag DF, but it really only makes sense * with callbacks, because otherwise there is no observable change * except that the server may fail where it would otherwise succeed. @@ -259,7 +270,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, } return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, - buf, NULL); + buf, &cb); } int64_t @@ -267,7 +278,18 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, size_t count, uint64...

This isn't quite finished because not all of the tests or examples have been updated, but it demonstrates an idea: Should we forget about the concept of having multiple connections managed under a single handle? In this patch there is a single ‘struct nbd_handle *’ which manages a single state machine and connection (and therefore no nbd_connection). To connect to a multi-conn server you must

...* sent to the server, so that NBD_CMD_DISC will be first in line. */ @@ -69,11 +64,6 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); if (id == -1) return -1; diff --git a/lib/rw.c b/lib/rw.c index f3adb71..95c002c 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -280,15 +280,6 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, { struct command_cb cb = { .completion = *completio...

...dle { * replies in server order. */ struct command *cmds_done; + struct command *cmds_done_tail; /* length (cmds_to_issue) + length (cmds_in_flight). */ int in_flight; diff --git a/lib/rw.c b/lib/rw.c index 3c2166f..680b81a 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -163,7 +163,7 @@ nbd_internal_command_common (struct nbd_handle *h, uint64_t offset, uint64_t count, void *data, struct command_cb *cb) { - struct command *cmd, *prev_cmd; + struct command *cmd; if (h->disconnect_request) { set_error (EINVAL, "cannot request mor...