Our construction of struct command_in_flight was ad hoc; most parameters were set in command_common(), but extent callbacks were done after the fact, and 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/disconnect.c b/lib/disconnect.c index bc43b4c..9706835 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -60,27 +60,17 @@ nbd_unlocked_shutdown (struct nbd_handle *h) int nbd_unlocked_aio_disconnect (struct nbd_connection *conn) { - struct command_in_flight *cmd; + int64_t id; - cmd = malloc (sizeof *cmd); - if (cmd == NULL) { - set_error (errno, "malloc"); + id = nbd_internal_command_common (conn, 0, NBD_CMD_DISC, 0, 0, NULL, + 0, NULL); + if (id == -1) return -1; - } - cmd->flags = 0; - cmd->type = NBD_CMD_DISC; - cmd->handle = conn->h->unique++; - cmd->offset = 0; - cmd->count = 0; - cmd->data = NULL; - - cmd->next = conn->cmds_to_issue; - conn->cmds_to_issue = cmd; /* 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 * the command struct to tell SEND_REQUEST not to add it to the * in-flight list. */ - return nbd_internal_run (conn->h, conn, cmd_issue); + return 0; } diff --git a/lib/internal.h b/lib/internal.h index 1f742da..de9b8bc 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -265,6 +265,13 @@ extern void nbd_internal_set_last_error (int errnum, char *error); extern int nbd_internal_errno_of_nbd_error (uint32_t error); extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type); +/* rw.c */ +extern int64_t nbd_internal_command_common (struct nbd_connection *conn, + uint16_t flags, uint16_t type, + uint64_t offset, uint64_t count, + void *data, int64_t id, + extent_fn extent); + /* socket.c */ struct socket *nbd_internal_socket_create (int fd); diff --git a/lib/rw.c b/lib/rw.c index 861ab67..8f6227d 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -241,10 +241,11 @@ nbd_unlocked_block_status (struct nbd_handle *h, return r == -1 ? -1 : 0; } -static struct command_in_flight * -command_common (struct nbd_connection *conn, - uint16_t flags, uint16_t type, - uint64_t offset, uint64_t count, void *data) +int64_t +nbd_internal_command_common (struct nbd_connection *conn, + uint16_t flags, uint16_t type, + uint64_t offset, uint64_t count, void *data, + int64_t id, extent_fn extent) { struct command_in_flight *cmd; @@ -255,7 +256,7 @@ command_common (struct nbd_connection *conn, if (count > MAX_REQUEST_SIZE) { set_error (ERANGE, "request too large: maximum request size is %d", MAX_REQUEST_SIZE); - return NULL; + return -1; } break; @@ -268,7 +269,7 @@ command_common (struct nbd_connection *conn, if (count > UINT32_MAX) { set_error (ERANGE, "request too large: maximum request size is %" PRIu32, UINT32_MAX); - return NULL; + return -1; } break; } @@ -276,7 +277,7 @@ command_common (struct nbd_connection *conn, cmd = calloc (1, sizeof *cmd); if (cmd == NULL) { set_error (errno, "calloc"); - return NULL; + return -1; } cmd->flags = flags; cmd->type = type; @@ -284,6 +285,8 @@ command_common (struct nbd_connection *conn, cmd->offset = offset; cmd->count = count; cmd->data = data; + cmd->extent_id = id; + cmd->extent_fn = extent; /* If structured replies were negotiated then we trust the server to * send back sufficient data to cover the whole buffer. It's tricky @@ -298,23 +301,18 @@ command_common (struct nbd_connection *conn, cmd->next = conn->cmds_to_issue; conn->cmds_to_issue = cmd; + if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) + return -1; - return cmd; + return cmd->handle; } int64_t nbd_unlocked_aio_pread (struct nbd_connection *conn, void *buf, size_t count, uint64_t offset) { - struct command_in_flight *cmd; - - cmd = command_common (conn, 0, NBD_CMD_READ, offset, count, buf); - if (!cmd) - return -1; - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) - return -1; - - return cmd->handle; + return nbd_internal_command_common (conn, 0, NBD_CMD_READ, offset, count, + buf, 0, NULL); } int64_t @@ -322,8 +320,6 @@ nbd_unlocked_aio_pwrite (struct nbd_connection *conn, const void *buf, size_t count, uint64_t offset, uint32_t flags) { - struct command_in_flight *cmd; - if (nbd_unlocked_read_only (conn->h) == 1) { set_error (EINVAL, "server does not support write operations"); return -1; @@ -340,33 +336,20 @@ nbd_unlocked_aio_pwrite (struct nbd_connection *conn, const void *buf, return -1; } - cmd = command_common (conn, flags, NBD_CMD_WRITE, offset, count, - (void *) buf); - if (!cmd) - return -1; - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) - return -1; - - return cmd->handle; + return nbd_internal_command_common (conn, flags, NBD_CMD_WRITE, offset, count, + (void *) buf, 0, NULL); } int64_t nbd_unlocked_aio_flush (struct nbd_connection *conn) { - struct command_in_flight *cmd; - if (nbd_unlocked_can_flush (conn->h) != 1) { set_error (EINVAL, "server does not support flush operations"); return -1; } - cmd = command_common (conn, 0, NBD_CMD_FLUSH, 0, 0, NULL); - if (!cmd) - return -1; - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) - return -1; - - return cmd->handle; + return nbd_internal_command_common (conn, 0, NBD_CMD_FLUSH, 0, 0, + NULL, 0, NULL); } int64_t @@ -374,8 +357,6 @@ nbd_unlocked_aio_trim (struct nbd_connection *conn, uint64_t count, uint64_t offset, uint32_t flags) { - struct command_in_flight *cmd; - if (nbd_unlocked_read_only (conn->h) == 1) { set_error (EINVAL, "server does not support write operations"); return -1; @@ -397,21 +378,14 @@ nbd_unlocked_aio_trim (struct nbd_connection *conn, return -1; } - cmd = command_common (conn, flags, NBD_CMD_TRIM, offset, count, NULL); - if (!cmd) - return -1; - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) - return -1; - - return cmd->handle; + return nbd_internal_command_common (conn, flags, NBD_CMD_TRIM, offset, count, + NULL, 0, NULL); } int64_t nbd_unlocked_aio_cache (struct nbd_connection *conn, uint64_t count, uint64_t offset) { - struct command_in_flight *cmd; - /* Actually according to the NBD protocol document, servers do exist * that support NBD_CMD_CACHE but don't advertize the * NBD_FLAG_SEND_CACHE bit, but we ignore those. @@ -421,13 +395,8 @@ nbd_unlocked_aio_cache (struct nbd_connection *conn, return -1; } - cmd = command_common (conn, 0, NBD_CMD_CACHE, offset, count, NULL); - if (!cmd) - return -1; - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) - return -1; - - return cmd->handle; + return nbd_internal_command_common (conn, 0, NBD_CMD_CACHE, offset, count, + NULL, 0, NULL); } int64_t @@ -435,8 +404,6 @@ nbd_unlocked_aio_zero (struct nbd_connection *conn, uint64_t count, uint64_t offset, uint32_t flags) { - struct command_in_flight *cmd; - if (nbd_unlocked_read_only (conn->h) == 1) { set_error (EINVAL, "server does not support write operations"); return -1; @@ -458,13 +425,8 @@ nbd_unlocked_aio_zero (struct nbd_connection *conn, return -1; } - cmd = command_common (conn, flags, NBD_CMD_WRITE_ZEROES, offset, count, NULL); - if (!cmd) - return -1; - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) - return -1; - - return cmd->handle; + return nbd_internal_command_common (conn, flags, NBD_CMD_WRITE_ZEROES, offset, + count, NULL, 0, NULL); } int64_t @@ -474,8 +436,6 @@ nbd_unlocked_aio_block_status (struct nbd_connection *conn, int64_t id, extent_fn extent) { - struct command_in_flight *cmd; - if (!conn->structured_replies) { set_error (ENOTSUP, "server does not support structured replies"); return -1; @@ -498,15 +458,6 @@ nbd_unlocked_aio_block_status (struct nbd_connection *conn, return -1; } - cmd = command_common (conn, flags, NBD_CMD_BLOCK_STATUS, offset, count, NULL); - if (!cmd) - return -1; - - cmd->extent_fn = extent; - cmd->extent_id = id; - - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) - return -1; - - return cmd->handle; + return nbd_internal_command_common (conn, flags, NBD_CMD_BLOCK_STATUS, offset, + count, NULL, id, extent); } -- 2.20.1