search for: conn_recv_full

...rc/connections.c @@ -600,6 +600,31 @@ send_newstyle_option_reply_info_export (struct connection *conn, return 0; } +/* Sub-function during _negotiate_handshake_newstyle, to uniformly handle + * a client hanging up on a message boundary. + */ +static int __attribute__ ((format (printf, 4, 5))) +conn_recv_full (struct connection *conn, void *buf, size_t len, + const char *fmt, ...) +{ + int r = conn->recv (conn, buf, len); + va_list args; + + if (r == -1) { + va_start (args, fmt); + nbdkit_verror (fmt, args); + va_end (args); + return -1; + } + if (r == 0) { + /* Dur...

...amp;export, sizeof export) == -1) { - nbdkit_error ("write: %m"); - return -1; - } - - return 0; -} - -/* Sub-function during _negotiate_handshake_newstyle, to uniformly handle - * a client hanging up on a message boundary. - */ -static int __attribute__ ((format (printf, 4, 5))) -conn_recv_full (struct connection *conn, void *buf, size_t len, - const char *fmt, ...) -{ - int r = conn->recv (conn, buf, len); - va_list args; - - if (r == -1) { - va_start (args, fmt); - nbdkit_verror (fmt, args); - va_end (args); - return -1; - } - if (r == 0) { - /* Dur...

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.

...(option)); return -1; } @@ -171,11 +167,11 @@ send_newstyle_option_reply_meta_context (struct connection *conn, /* Sub-function during negotiate_handshake_newstyle, to uniformly handle * a client hanging up on a message boundary. */ -static int __attribute__ ((format (printf, 4, 5))) -conn_recv_full (struct connection *conn, void *buf, size_t len, - const char *fmt, ...) +static int __attribute__ ((format (printf, 3, 4))) +conn_recv_full (void *buf, size_t len, const char *fmt, ...) { - int r = conn->recv (conn, buf, len); + struct connection *conn = GET_CONN; + int r = c...

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.

...break; + case NBD_OPT_LIST_META_CONTEXT: + case NBD_OPT_SET_META_CONTEXT: + { + uint32_t opt_index; + uint32_t exportnamelen; + uint32_t nr_queries; + uint32_t querylen; + const char *what; + + optname = name_of_nbd_opt (option); + if (conn_recv_full (conn, data, optlen, "read: %s: %m", optname) == -1) + return -1; + + if (!conn->structured_replies) { + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + continue; + } + +...

...2 (reply); fixed_new_option_reply.replylen = htobe32 (sizeof export); export.info = htobe16 (info); - 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)...

...ere we changed things to allow a client to request an explicit export name of "" yet still connect to an oldstyle server in that case. > +++ b/server/protocol-handshake-newstyle.c > @@ -274,11 +274,17 @@ negotiate_handshake_newstyle_options (struct connection *conn) > if (conn_recv_full (conn, data, optlen, > "read: %s: %m", name_of_nbd_opt (option)) == -1) > return -1; > - /* Apart from printing it, ignore the export name. */ > + /* Print the export name and save it in the connection. */ > data[optlen]...

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.

...-334,7 +344,7 @@ negotiate_handshake_newstyle_options (void) uint64_t exportsize; struct backend *b; - for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) { + for (nr_options = MAX_NR_OPTIONS; nr_options > 0; --nr_options) { CLEANUP_FREE char *data = NULL; if (conn_recv_full (&new_option, sizeof new_option, @@ -431,11 +441,22 @@ negotiate_handshake_newstyle_options (void) continue; } - /* Send back the exportname list. */ - debug ("newstyle negotiation: %s: advertising exports", - name_of_nbd_opt (option)); - if...

...-oldstyle.c | 3 +++ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 2480d7a3..878fe53f 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -198,7 +198,7 @@ conn_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_newst...

...shake-newstyle.c @@ -233,6 +233,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) char data[MAX_OPTION_LENGTH+1]; struct new_handshake_finish handshake_finish; const char *optname; + int r; for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) { if (conn_recv_full (conn, &new_option, sizeof new_option, @@ -481,6 +482,20 @@ negotiate_handshake_newstyle_options (struct connection *conn) debug ("newstyle negotiation: %s: client requested structured replies", name_of_nbd_opt (option)); + r = backend->can_sr (backend, c...

...iff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 9ddc319..e1301a0 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -274,11 +274,17 @@ negotiate_handshake_newstyle_options (struct connection *conn) if (conn_recv_full (conn, data, optlen, "read: %s: %m", name_of_nbd_opt (option)) == -1) return -1; - /* Apart from printing it, ignore the export name. */ + /* Print the export name and save it in the connection. */ data[optlen] = '\0'; - de...

...eturn NULL; + } + } + return p->plugin.open (readonly); } diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-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) {...

...at most once per connection. I don't think so - backend_reopen will call plugin_open a second time. As a test I added assert (conn->exportname == NULL) before this line 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, > >+...

...le > + * because of the lifetime issue. > + */ > + if (conn->exportname == NULL) { Can't we assert(!conn->exportname) at this point? After 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...

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

...cv; @@ -905,6 +906,26 @@ _negotiate_handshake_newstyle_options (struct connection *conn) break; + case NBD_OPT_STRUCTURED_REPLY: + if (optlen != 0) { + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + if (conn_recv_full (conn, data, optlen, + "read: %s: %m", name_of_nbd_opt (option)) == -1) + return -1; + continue; + } + + debug ("newstyle negotiation: %s: client requested structured replies", + name_of_nbd_opt (option)); + +...

...structured replies are disabled", + name_of_nbd_opt (option)); + break; + } + if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) return -1; @@ -499,6 +510,9 @@ negotiate_handshake_newstyle_options (struct connection *conn) if (conn_recv_full (conn, data, optlen, "read: %s: %m", optname) == -1) return -1; + /* Note that we support base:allocation whether or not the plugin + * supports can_extents. + */ if (!conn->structured_replies) { if (send_newstyle_option_reply (con...