search for: nbd_cmd_write

...= exportsize; } -static int +static bool validate_request (struct connection *conn, uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t count, uint32_t *error) { int r; + /* Readonly connection? */ + if (conn->readonly && + (cmd == NBD_CMD_WRITE || + cmd == NBD_CMD_TRIM || cmd == NBD_CMD_WRITE_ZEROES)) { + nbdkit_error ("invalid request: write request on readonly connection"); + *error = EROFS; + return false; + } + /* Validate cmd, offset, count. */ switch (cmd) { case NBD_CMD_READ: case NBD_CMD_WRITE...

When facing a malicious client that is sending bogus NBD_CMD_READ, we should make sure that we never end up in a situation where we could try to treat the tail from a command that we diagnosed as bad as being further commands. Eric Blake (2): connections: Report mid-message EOF as fatal connections: Hang up early on insanely large WRITE requests src/connections.c | 35

I'm not sure if this is worth pursuing. On paper, it makes sense (if we know we have multiple commands batched to send over the wire, AND those commands are short in length, we might as well use MSG_MORE), but the measurement numbers with it applied might just be in the noise. Eric Blake (2): examples: Enhance access patterns of threaded-reads-and-writes states: Another use for MSG_MORE

On 11/17/2017 12:28 PM, Eric Blake wrote: >> There's nothing wrong with this patch, but it might be easier to use >> an attribute((cleanup)) handler to deal with the unlocking. See these >> links for how we do it in libguestfs: > > Oh cool! Yes, that looks nicer. Although the diffstat for doing so is > larger, because it requires adding to new sub-function {}

...sue-command.c index cce43d7..5d2a7e6 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -42,7 +42,7 @@ h->request.count = htobe32 ((uint32_t) cmd->count); h->wbuf = &h->request; h->wlen = sizeof (h->request); - if (cmd->type == NBD_CMD_WRITE) + if (cmd->type == NBD_CMD_WRITE || cmd->next) h->wflags = MSG_MORE; SET_NEXT_STATE (%SEND_REQUEST); return 0; @@ -70,6 +70,8 @@ if (cmd->type == NBD_CMD_WRITE) { h->wbuf = cmd->data; h->wlen = cmd->count; + if (cmd->next && cmd->co...

While it may be counterintuitive at first, the introduction of NBD_CMD_WRITE_ZEROES and NBD_CMD_BLOCK_STATUS has caused a performance regression in qemu [1], when copying a sparse file. When the destination file must contain the same contents as the source, but it is not known in advance whether the destination started life with all zero content, then there are cases where...

...<= exportsize && offset + count <= exportsize; -} - static bool validate_request (struct connection *conn, uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, uint32_t *error) { - int r; - - /* Readonly connection? */ - if (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_TRIM || - cmd == NBD_CMD_WRITE_ZEROES) { - r = backend_can_write (backend, conn); - assert (r >= 0); /* Guaranteed by eflags computation */ - if (!r) { - nbdkit_error ("invalid request: %s: write request on readonly connection", -...

...ric Blake for finding the bug. > --- > server/internal.h | 1 + > server/protocol.c | 16 +++++++++------- > server/threadlocal.c | 37 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 47 insertions(+), 7 deletions(-) > > if (cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) { > - buf = malloc (count); > + buf = threadlocal_buffer ((size_t) count); > if (buf == NULL) { > - out_of_memory: > - perror ("malloc"); > error = ENOMEM; Old code called perror() when nbdkit_extents_new() failed... >...

On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote: > Fixes the fact that clients could not request the maximum string > length except with NBD_OPT_EXPORT_LEN. Updates the testsuite to > match. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > server/protocol-handshake-newstyle.c | 12 +++++++----- > tests/test-long-name.sh | 10

...p; offset + count <= exportsize; -} - -static bool -validate_request (struct connection *conn, - uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, - uint32_t *error) -{ - /* Readonly connection? */ - if (conn->readonly && - (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_TRIM || - cmd == NBD_CMD_WRITE_ZEROES)) { - nbdkit_error ("invalid request: %s: write request on readonly connection", - name_of_nbd_cmd (cmd)); - *error = EROFS; - return false; - } - - /* Validate cmd, offset, count. */ - switch (cmd)...

...ize of 2^12 (4,096), and a maximum block size > -that is effectively unlimited (0xffffffff, or the export size if that > -is smaller), while a client desiring maximum interoperability SHOULD > -constrain its requests to a minimum block size of 2^9 (512), and limit > -`NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum block size of > -2^25 (33,554,432). A server that wants to enforce block sizes other > -than the defaults specified here MAY refuse to go into transmission > -phase with a client that uses `NBD_OPT_EXPORT_NAME` (via a hard > -disconnect) or which uses `NBD_OPT_GO...

On Fri, Mar 22, 2019 at 6:43 PM Eric Blake <eblake@redhat.com> wrote: > While it may be counterintuitive at first, the introduction of > NBD_CMD_WRITE_ZEROES and NBD_CMD_BLOCK_STATUS has caused a performance > regression in qemu [1], when copying a sparse file. When the > destination file must contain the same contents as the source, but it > is not known in advance whether the destination started life with all > zero content, then th...

These are a couple of patches in preparation for the Block Status implementation. While the patches (especially the second one) are very large they are really just elementary code motion. Rich.

...ng attempted for a fast wipe, and >> get an immediate failure if the zero request would otherwise take the >> same time as a write. Conversely, if the client is not performing a >> pre-initialization pass, it is still more efficient in terms of >> networking traffic to send NBD_CMD_WRITE_ZERO requests where the >> server implements the fallback to the slower write, than it is for the >> client to have to perform the fallback to send NBD_CMD_WRITE with a >> zeroed buffer. > > How are you going to finally use it in qemu-img convert? It's already in use t...

...__attribute__((packed)); + /* New-style handshake server reply when using NBD_OPT_EXPORT_NAME. * Modern clients use NBD_OPT_GO instead of this. */ @@ -199,10 +212,12 @@ extern const char *name_of_nbd_cmd (int); #define NBD_CMD_FLUSH 3 #define NBD_CMD_TRIM 4 #define NBD_CMD_WRITE_ZEROES 6 +#define NBD_CMD_BLOCK_STATUS 7 extern const char *name_of_nbd_cmd_flag (int); #define NBD_CMD_FLAG_FUA (1<<0) #define NBD_CMD_FLAG_NO_HOLE (1<<1) +#define NBD_CMD_FLAG_REQ_ONE (1<<3) /* Error codes (previously errno). * See http://git.qemu.org/...

This bug was found by Eric Blake. In the .pread method we allocate a buffer in the server and pass it to the plugin. The plugin is supposed to fill it with data. The buffer was uninitialized so initially contained random heap data, but that's OK provided the plugin fully overwrote it with data. All correctly written plugins ought to do this, however there is the possibility of an

...mpiler for nbdkit, just to get nicer code? Another argument in favor of making it mandatory - it is VERY easy to write a client that would make nbdkit run out of memory if nbdkit is compiled without cleanup support. connections.c uses CLEANUP_FREE char *buf, which can be allocated at up to 64M per NBD_CMD_WRITE; all a client has to do is send the NBD_CMD_WRITE header, drop the connection, and reconnect, and if buf is not auto-freed, nbdkit will leak quite rapidly. So that's the approach I'll submit in my next round of patch postings. -- Eric Blake, Principal Software Engineer Red Hat, Inc....

...l.c b/server/protocol.c index 54d8adb..4d67392 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -658,10 +658,10 @@ protocol_recv_request_send_reply (struct connection *conn) /* Allocate the data buffer used for either read or write requests. */ if (cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) { - buf = malloc (count); + buf = calloc (1, count); if (buf == NULL) { out_of_memory: - perror ("malloc"); + perror ("calloc"); error = ENOMEM; if (cmd == NBD_CMD_WRITE && skip_over_write_buffer (conn...

...& (flags & NBD_CMD_FLAG_FUA); > int err = 0; > + int r; > > /* Clear the error, so that we know if the plugin calls > * nbdkit_set_error() or relied on errno. */ > @@ -246,7 +271,7 @@ handle_request (struct connection *conn, > break; > > case NBD_CMD_WRITE: > - if (fua) > + if (flags & NBD_CMD_FLAG_FUA) > f |= NBDKIT_FLAG_FUA; So don't we need to keep the backend_can_fua() test here and later in this function? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming a...

...ommand_in_flight *cmd; - switch (send_from_wbuf (conn)) { case -1: SET_NEXT_STATE (%.DEAD); return -1; - case 0: - assert (conn->cmds_in_flight != NULL); - cmd = conn->cmds_in_flight; - assert (cmd->handle == be64toh (conn->sbuf.request.handle)); - if (cmd->type == NBD_CMD_WRITE) { - conn->wbuf = cmd->data; - conn->wlen = cmd->count; - SET_NEXT_STATE (%SEND_WRITE_PAYLOAD); - } - else - SET_NEXT_STATE (%.READY); + case 0: SET_NEXT_STATE (%PREPARE_WRITE_PAYLOAD); } return 0; + ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD: + struct comma...