[Libguestfs] [PATCH nbdkit v2] protocol: Implement NBD_OPT_GO.
Eric Blake
eblake at redhat.com
Mon Aug 6 13:56:34 UTC 2018
On 08/06/2018 05:45 AM, Richard W.M. Jones wrote:
> ---
> src/connections.c | 233 +++++++++++++++++++++++++++++++++++++---------
> src/protocol.h | 27 ++++--
> 2 files changed, 209 insertions(+), 51 deletions(-)
>
> @@ -701,6 +754,101 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
> }
> break;
>
> + case NBD_OPT_GO:
> + if (conn->recv (conn, data, optlen) == -1) {
> + nbdkit_error ("read: %m");
> + return -1;
> + }
> +
> + if (optlen < 6) { /* 32 bit export length + 16 bit nr info */
> + debug ("newstyle negotiation: NBD_OPT_GO option length < 6");
> +
> + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
> + == -1)
> + return -1;
> + continue;
> + }
> +
> + {
> + uint32_t exportnamelen;
> + uint16_t nrinfos;
> + uint16_t info;
> + size_t i;
> + CLEANUP_FREE char *requested_exportname = NULL;
> +
> + /* Validate the name length and number of INFO requests. */
> + memcpy (&exportnamelen, &data[0], 4);
> + exportnamelen = be32toh (exportnamelen);
> + if (exportnamelen > optlen-6 /* NB optlen >= 6, see above */) {
Spacing around '-'
> + debug ("newstyle negotiation: NBD_OPT_GO: export name too long");
> + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
> + == -1)
> + return -1;
> + continue;
> + }
> + memcpy (&nrinfos, &data[exportnamelen+4], 2);
Spacing around '+'
> + nrinfos = be16toh (nrinfos);
> + if (optlen != 4 + exportnamelen + 2 + 2*nrinfos) {
Spacing around '*'
> + debug ("newstyle negotiation: NBD_OPT_GO: "
> + "number of information requests incorrect");
> + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
> + == -1)
> + return -1;
> + continue;
> + }
> +
> + /* As with NBD_OPT_EXPORT_NAME we print the export name and then
> + * ignore it.
> + */
> + requested_exportname = malloc (exportnamelen+1);
Spacing around '+'
> + if (requested_exportname == NULL) {
> + nbdkit_error ("malloc: %m");
> + return -1;
> + }
> + memcpy (requested_exportname, &data[4], exportnamelen);
> + requested_exportname[exportnamelen] = '\0';
> + debug ("newstyle negotiation: NBD_OPT_GO: "
> + "client requested export '%s' (ignored)",
> + requested_exportname);
> +
> + /* The spec is confusing, but it is required that we send back
> + * NBD_INFO_EXPORT, even if the client did not request it!
> + * qemu client in particular does not request this, but will
> + * fail if we don't send it.
> + */
> + if (get_size_and_eflags (conn) == -1)
> + return -1;
> + if (send_newstyle_option_reply_info_export (conn, option,
> + NBD_REP_INFO,
> + NBD_INFO_EXPORT) == -1)
> + return -1;
> +
> + /* For now we ignore all other info requests (but we must
> + * ignore NBD_INFO_EXPORT if it was requested, because we
> + * replied already above). Therefore this loop doesn't do
> + * much at the moment.
> + */
> + for (i = 0; i < nrinfos; ++i) {
> + memcpy (&info, &data[4+exportnamelen+2+i*2], 2);
Spacing.
> + info = be16toh (info);
> + switch (info) {
> + case NBD_INFO_EXPORT: /* ignore - reply sent above */ break;
> + default:
> + debug ("newstyle negotiation: NBD_OPT_GO: "
> + "ignoring NBD_INFO_* request %u", (unsigned) info);
> + break;
> + }
> + }
> + }
> +
> + /* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK
> + * or ERROR packet.
> + */
> + if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
> + return -1;
> +
> + break;
> +
> default:
> /* Unknown option. */
> if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1)
> @@ -712,10 +860,10 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
> }
>
> /* Note, since it's not very clear from the protocol doc, that the
> - * client must send NBD_OPT_EXPORT_NAME last, and that ends option
> - * negotiation.
> + * client must send NBD_OPT_EXPORT_NAME or NBD_OPT_GO last, and
> + * that ends option negotiation.
> */
> - if (option == NBD_OPT_EXPORT_NAME)
> + if (option == NBD_OPT_EXPORT_NAME || option == NBD_OPT_GO)
> break;
NBD_OPT_EXPORT_NAME _always_ ends negotiation, since it has no error
response. But NBD_OPT_GO only ends negotiation if you replied with an
ACK. On the other hand, it looks like the code above only used 'break'
when sending ACK, and used 'continue' for all error replies, so looks
like you are good here.
> }
>
> @@ -733,6 +881,38 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
> return -1;
> }
>
> + /* If the last option was NBD_OPT_GO then we're all done. */
> + if (option == NBD_OPT_GO)
> + ;
> + /* Else if the last option was NBD_OPT_EXPORT_NAME then we have to
> + * finish the handshake by sending handshake_finish.
> + */
> + else if (option == NBD_OPT_EXPORT_NAME) {
> + if (get_size_and_eflags (conn) == -1)
> + return -1;
> +
> + memset (&handshake_finish, 0, sizeof handshake_finish);
> + handshake_finish.exportsize = htobe64 (conn->exportsize);
> + handshake_finish.eflags = htobe16 (conn->eflags);
> +
> + if (conn->send (conn,
> + &handshake_finish,
> + (conn->cflags & NBD_FLAG_NO_ZEROES)
> + ? offsetof (struct new_handshake_finish, zeroes)
> + : sizeof handshake_finish) == -1) {
> + nbdkit_error ("write: %m");
> + return -1;
> + }
> + }
> + /* The client is buggy. The last option must be NBD_OPT_GO or
> + * NBD_OPT_EXPORT_NAME.
> + */
> + else {
This else clause is unreachable. You can only exit the loop by too many
iterations (checked above), or by the post-switch break on just GO and
EXPORT_NAME.
> + nbdkit_error ("client options list didn't finish with NBD_OPT_GO "
> + "or NBD_OPT_EXPORT_NAME");
> + return -1;
So instead of this unreachable error message, you could just abort().
> + }
> +
> return 0;
> }
>
> @@ -741,11 +921,6 @@ _negotiate_handshake_newstyle (struct connection *conn)
> {
> struct new_handshake handshake;
> uint16_t gflags;
> - uint32_t cflags;
> - struct new_handshake_finish handshake_finish;
> - int64_t r;
> - uint64_t exportsize;
> - uint16_t eflags;
>
> gflags = NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES;
>
> @@ -761,15 +936,15 @@ _negotiate_handshake_newstyle (struct connection *conn)
> }
>
> /* Client now sends us its 32 bit flags word ... */
> - if (conn->recv (conn, &cflags, sizeof cflags) == -1) {
> + if (conn->recv (conn, &conn->cflags, 4) == -1) {
The sizeof looks nicer than a magic number 4.
So, just style and dead code, no major issues with the logic changes.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
More information about the Libguestfs
mailing list