search for: protocol_recv_request_send_reply

...,6 +183,7 @@ struct connection { bool can_multi_conn; bool using_tls; bool structured_replies; + bool meta_context_base_allocation; int sockin, sockout; connection_recv_function recv; @@ -219,6 +220,12 @@ extern int protocol_handshake_newstyle (struct connection *conn) extern int protocol_recv_request_send_reply (struct connection *conn) __attribute__((__nonnull__ (1))); +/* The context ID of base:allocation. As far as I can tell it doesn't + * matter what this is as long as nbdkit always returns the same + * number. + */ +#define base_allocation_id 1 + /* crypto.c */ #define root_tls_certifica...

...,6 +183,7 @@ struct connection { bool can_multi_conn; bool using_tls; bool structured_replies; + bool meta_context_base_allocation; int sockin, sockout; connection_recv_function recv; @@ -219,6 +220,12 @@ extern int protocol_handshake_newstyle (struct connection *conn) extern int protocol_recv_request_send_reply (struct connection *conn) __attribute__((__nonnull__ (1))); +/* The context ID of base:allocation. As far as I can tell it doesn't + * matter what this is as long as nbdkit always returns the same + * number. + */ +#define base_allocation_id 1 + /* crypto.c */ #define root_tls_certifica...

GCC 9.0.1 isn't smart enough to work out that buf is initialized on all the relevant paths that lead to backend->pwrite being called. By initializing it we can avoid the warning. protocol.c: In function ‘protocol_recv_request_send_reply’: protocol.c:241:9: error: ‘buf’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 241 | if (backend->pwrite (backend, conn, buf, count, offset, f, &err) == -1) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ protocol.c:614:9: note: ‘...

...cal_get_error (void); +extern void *threadlocal_buffer (size_t size); /* Declare program_name. */ #if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME == 1 diff --git a/server/protocol.c b/server/protocol.c index 3f89f6d..9e8eea5 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -611,7 +611,7 @@ protocol_recv_request_send_reply (struct connection *conn) uint16_t cmd, flags; uint32_t magic, count, error = 0; uint64_t offset; - CLEANUP_FREE char *buf = NULL; + char *buf; CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents = NULL; /* Read the request packet. */ @@ -656,12 +656,12 @@ protocol_recv_request_s...

Version 1 was here: https://www.redhat.com/archives/libguestfs/2019-April/msg00144.html Version 2 makes a couple of much larger changes: The OCaml patch changes the API of the pread method so it matches what other language bindings are already doing, ie. get the language plugin to return a newly allocated buffer, check it is long enough, copy out the data. The server patch implements a

...ut_of_memory: > - perror ("malloc"); > error = ENOMEM; Old code called perror() when nbdkit_extents_new() failed... > if (cmd == NBD_CMD_WRITE && > skip_over_write_buffer (conn->sockin, count) < 0) > @@ -673,8 +673,10 @@ protocol_recv_request_send_reply (struct connection *conn) > /* Allocate the extents list for block status only. */ > if (cmd == NBD_CMD_BLOCK_STATUS) { > extents = nbdkit_extents_new (offset, conn->exportsize); > - if (extents == NULL) > - goto out_of_memory; > + if (extents...

...t debug.c:91 #3 0x0000000000406507 in backend_pread (b=0x9f07040, buf=0x9f80dc0, count=512, offset=0, flags=0, err=0x7fc4aaa1fa78) at backend.c:482 #4 0x0000000000411f7e in handle_request (cmd=0, flags=0, offset=0, count=512, buf=0x9f80dc0, extents=0x0) at protocol.c:241 #5 0x00000000004131c9 in protocol_recv_request_send_reply () at protocol.c:713 #6 0x00000000004081e7 in connection_worker (data=0x9ee9ff0) at connections.c:123 #7 0x00007fc4b9d293f9 in start_thread () from /lib64/libpthread.so.0 #8 0x00007fc4b9c55b23 in clone () from /lib64/libc.so.6 Thread 7 (Thread 0x7fc4aba22640 (LWP 109867)): #0 0x00007fc4b9c5079...

...tribute__((__nonnull__ (1))); +extern int protocol_handshake_oldstyle (void); /* protocol-handshake-newstyle.c */ -extern int protocol_handshake_newstyle (struct connection *conn) - __attribute__((__nonnull__ (1))); +extern int protocol_handshake_newstyle (void); /* protocol.c */ -extern int protocol_recv_request_send_reply (struct connection *conn) - __attribute__((__nonnull__ (1))); +extern int protocol_recv_request_send_reply (void); /* The context ID of base:allocation. As far as I can tell it doesn't * matter what this is as long as nbdkit always returns the same @@ -268,9 +258,7 @@ extern int protocol...

v1 was here: https://www.redhat.com/archives/libguestfs/2020-February/msg00081.html v2 replaces struct connection *conn = GET_CONN; with GET_CONN; which sets conn implicitly and asserts that it is non-NULL. If we actually want to test if conn is non-NULL or behave differently, then you must use threadlocal_get_conn() instead, and some existing uses do that. Rich.

The third patch is a large but mechanical change which gets rid of passing around struct connection * entirely within the server, preferring instead to reference the connection through thread-local storage. I hope this is a gateway to simplifying other parts of the code. Rich.

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

..., } /* Send the error. */ - error_data.error = htobe32 (nbd_errno (error)); + error_data.error = htobe32 (nbd_errno (error, flags & NBD_CMD_FLAG_DF)); error_data.len = htobe16 (0); r = conn->send (conn, &error_data, sizeof error_data); if (r == -1) { @@ -737,7 +740,8 @@ protocol_recv_request_send_reply (struct connection *conn) extents); } else - return send_structured_reply_error (conn, request.handle, cmd, error); + return send_structured_reply_error (conn, request.handle, cmd, flags, +...

...ay be supplied by others. Credit: Eric Blake for finding the bug. --- server/protocol.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/protocol.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&...

...atus (conn, -1); > + } > + } Where does the list terminate once you send the final extent that overlaps the end of the original request? (If you implement my idea of tracking a maximum offset in nbdkit_extents in patch 1, this may be picked up automatically) > @@ -498,15 +605,23 @@ protocol_recv_request_send_reply (struct connection *conn) > } > > /* Currently we prefer to send simple replies for everything except > - * where we have to (ie. NBD_CMD_READ when structured_replies have > - * been negotiated). However this prevents us from sending > - * human-readable error messa...

Not sure what version we're up to, but this reimplements extents using the new simpler structure described in this thread: https://www.redhat.com/archives/libguestfs/2019-March/msg00077.html I also fixed most of the things that Eric pointed out in the previous review, although I need to go back over his replies and check I've got everything. This needs a bit more testing. However the

I want to post this but mainly for discussion and early review. It's not safe for these patches to all go upstream yet (because not all filters have been checked/adjusted), but if any patches were to go upstream then probably 1 & 2 only are safe. File, VDDK, memory and data plugins all work, although I have only done minimal testing on them. The current tests, such as they are, all

...t exportsize = conn->exportsize; + uint64_t exportsize = backend_get_size (backend, conn); + assert (exportsize <= INT64_MAX); /* Guaranteed by negotiation phase */ return count > 0 && offset <= exportsize && offset + count <= exportsize; } @@ -705,7 +706,7 @@ protocol_recv_request_send_reply (struct connection *conn) /* Allocate the extents list for block status only. */ if (cmd == NBD_CMD_BLOCK_STATUS) { - extents = nbdkit_extents_new (offset, conn->exportsize); + extents = nbdkit_extents_new (offset, backend_get_size (backend, conn)); if (extents == NUL...

I'm not sure exactly which version we're up to, but let's say it's version 4. I'm a lot happier with this version: - all filters have been reviewed and changed where I think that's necessary - can_extents is properly defined and implemented now - NBD protocol is followed - I believe it addresses all previous review points where possible The "only" thing

...newly written plugins will have the documentation of that particular aspect to make their decision on whether to opt-out. > diff --git a/server/protocol.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); Definitely a safety fix for READ, but isn't this...

...*/ > - error_data.error = htobe32 (nbd_errno (error)); > + error_data.error = htobe32 (nbd_errno (error, flags & NBD_CMD_FLAG_DF)); > error_data.len = htobe16 (0); > r = conn->send (conn, &error_data, sizeof error_data); > if (r == -1) { > @@ -737,7 +740,8 @@ protocol_recv_request_send_reply (struct connection *conn) > extents); > } > else > - return send_structured_reply_error (conn, request.handle, cmd, error); > + return send_structured_reply_error (conn, request.handle, cmd, flags, > +...