search for: finish_newstyle_opt

...; - export.exportsize = htobe64 (conn->exportsize); + export.exportsize = htobe64 (exportsize); export.eflags = htobe16 (conn->eflags); if (conn->send (conn, @@ -201,7 +201,7 @@ conn_recv_full (struct connection *conn, void *buf, size_t len, * in that function. */ static int -finish_newstyle_options (struct connection *conn) +finish_newstyle_options (struct connection *conn, uint64_t *exportsize) { int64_t r; @@ -213,7 +213,7 @@ finish_newstyle_options (struct connection *conn) "(%" PRIi64 ")", r); return -1; } - conn->exportsize = (u...

...nn_recv_full (struct connection *conn, void *buf, size_t len, /* Sub-function of negotiate_handshake_newstyle_options below. It * must be called on all non-error paths out of the options for-loop - * in that function. + * in that function, and must not cause any wire traffic. */ static int finish_newstyle_options (struct connection *conn, uint64_t *exportsize) @@ -322,7 +322,9 @@ negotiate_handshake_newstyle_options (struct connection *conn) if (check_export_name (conn, option, data, optlen, optlen, true) == -1) return -1; - /* We have to finish the handshake by sending handshake_f...

Since the v1 series (0/4, at [1]), I've applied patches 1 and 2, rewritten patch 3 [Forbid NUL in export and context names] into patch 4 here, patch 4 there turned into patch 6 here, and everything else here is new. [1]https://www.redhat.com/archives/libguestfs/2019-September/msg00180.html I don't know if there is a handy reusable function for checking whether a string contains valid

...-handshake-newstyle.c index fa44f253..0ed3c7c7 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char *fmt, ...) * in that function, and must not cause any wire traffic. */ static int -finish_newstyle_options (uint64_t *exportsize) +finish_newstyle_options (uint64_t *exportsize, + const char *exportname_in, uint32_t exportnamelen) { GET_CONN; - if (protocol_common_open (exportsize, &conn->eflags) == -1) + /* Since the exportname string passed here comes directl...

...and it crashed in tests/test-retry.sh. > >+++ b/server/protocol-handshake-newstyle.c > >@@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char *fmt, ...) > > * in that function, and must not cause any wire traffic. > > */ > > static int > >-finish_newstyle_options (uint64_t *exportsize) > >+finish_newstyle_options (uint64_t *exportsize, > >+ const char *exportname_in, uint32_t exportnamelen) > > { > > GET_CONN; > >- if (protocol_common_open (exportsize, &conn->eflags) == -1) > >+ /*...

...ter all, we only ever call .open at most once per connection. > +++ b/server/protocol-handshake-newstyle.c > @@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char *fmt, ...) > * in that function, and must not cause any wire traffic. > */ > static int > -finish_newstyle_options (uint64_t *exportsize) > +finish_newstyle_options (uint64_t *exportsize, > + const char *exportname_in, uint32_t exportnamelen) > { > GET_CONN; > > - if (protocol_common_open (exportsize, &conn->eflags) == -1) > + /* Since the expor...

...ffer. */ - debug ("client closed input socket, closing connection"); - return -1; - } - return r; -} - -/* Sub-function of _negotiate_handshake_newstyle_options below. It - * must be called on all non-error paths out of the options for-loop - * in that function. - */ -static int -finish_newstyle_options (struct connection *conn) -{ - int64_t r; - - r = backend->get_size (backend, conn); - if (r == -1) - return -1; - if (r < 0) { - nbdkit_error (".get_size function returned invalid value " - "(%" PRIi64 ")", r); - return -1; - }...

...conn->exportname = malloc (optlen+1); + if (conn->exportname == NULL) { + nbdkit_error ("malloc: %m"); + return -1; + } + memcpy (conn->exportname, data, optlen+1); /* We have to finish the handshake by sending handshake_finish. */ if (finish_newstyle_options (conn, &exportsize) == -1) @@ -413,19 +419,19 @@ negotiate_handshake_newstyle_options (struct connection *conn) continue; } - /* As with NBD_OPT_EXPORT_NAME we print the export name and then - * ignore it. + /* As with NBD_OPT_EXPORT_NAME we print...

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.

...t;recv (conn, buf, len); + struct connection *conn = GET_CONN; + int r = conn->recv (buf, len); va_list args; if (r == -1) { @@ -198,9 +194,11 @@ conn_recv_full (struct connection *conn, void *buf, size_t len, * in that function, and must not cause any wire traffic. */ static int -finish_newstyle_options (struct connection *conn, uint64_t *exportsize) +finish_newstyle_options (uint64_t *exportsize) { - if (protocol_common_open (conn, exportsize, &conn->eflags) == -1) + struct connection *conn = GET_CONN; + + if (protocol_common_open (exportsize, &conn->eflags) == -1) retu...

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.

This is the sort of thing I had in mind for option (1) here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html It does reveal that the way we currently list exports is naive to say the least ... Rich.

...xed_new_option_reply_meta_context context; + struct nbd_fixed_new_option_reply fixed_new_option_reply; + struct nbd_fixed_new_option_reply_meta_context context; const size_t namelen = strlen (name); debug ("newstyle negotiation: %s: replying with %s id %d", @@ -213,13 +213,13 @@ finish_newstyle_options (struct connection *conn, uint64_t *exportsize) static int negotiate_handshake_newstyle_options (struct connection *conn) { - struct new_option new_option; + struct nbd_new_option new_option; size_t nr_options; uint64_t version; uint32_t option; uint32_t optlen; char data[MA...

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.

It's easy to use the sh script to demonstrate that nbdkit is inefficiently calling into .get_size, .can_fua, and friends more than necessary. We've also commented on the list in the past that it would be nice to ensure that when filters call into next_ops, they are not violating constraints (as we've have to fix several bugs in the past where we did not have such checking to protect

The previous incomplete patch was here: https://www.redhat.com/archives/libguestfs/2019-September/msg00049.html based on earlier discussion here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html In v2: - The previous patch was incomplete. This version completes it by adding tests and extending nbdkit-sh-plugin. - nbdkit_export_name now returns NULL for error,

With some crufty sed scripts we can generate functions that map from NBD protocol flags (eg. NBD_CMD_READ) to strings ("NBD_CMD_READ"). This works on GNU sed and with FreeBSD, also with GNU sed's --posix option, so I guess the sed code is POSIX-compatible. Rich.

The first one is the nastiest - it is an assertion failure caused by a spec-compliant client and introduced by our security fix that was released in 1.14.1. Eric Blake (4): server: Fix regression for NBD_OPT_INFO before NBD_OPT_GO server: Fix back-to-back SET_META_CONTEXT server: Forbid NUL in export and context names server: Fix OPT_GO on different export than SET_META_CONTEXT

...ocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 41b2a6e4..946060ac 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -478,9 +478,9 @@ negotiate_handshake_newstyle_options (void) * disconnecting. */ if (finish_newstyle_options (&exportsize) == -1) { - if (backend_finalize (backend) == -1) + if (backend_finalize (top) == -1) return -1; - backend_close (backend); + backend_close (top); if (send_newstyle_option_reply (option, NBD_REP_ERR_UNKNOWN) == -1)...

We should have only one NBD protocol file. Let's make nbdkit's version the canonical one, and use it in libnbd. Rich.