Re: [Qemu-devel] [PATCH v3 10/13] nbd/client: refactor nbd_receive_starttls

[Eric Blake]

On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> Split out nbd_receive_simple_option to be reused for structured reply

s/receive/request/, but see [1]

> option.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  nbd/client.c     | 64 
> ++++++++++++++++++++++++++++++++++++++++----------------
>  nbd/trace-events |  7 ++++---
>  2 files changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index cd5a2c80ac..c8702a80b1 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -540,35 +540,63 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
>      }
>  }
>  
> -static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> -                                        QCryptoTLSCreds *tlscreds,
> -                                        const char *hostname, Error **errp)
> +/* nbd_request_simple_option
> + * return 1 for successful negotiation,
> + *        0 if operation is unsupported,
> + *        -1 with errp set for any other error
> + */
> +static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
>  {
>      nbd_opt_reply reply;

Unrelated, but a potential cleanup for a future patch: we really should
be using our preferred naming scheme, where this would be NBDOptReply or
similar.

> -    QIOChannelTLS *tioc;
> -    struct NBDTLSHandshakeData data = { 0 };
>  
> -    trace_nbd_receive_starttls_request();
> -    if (nbd_send_option_request(ioc, NBD_OPT_STARTTLS, 0, NULL, errp) < 0) {
> -        return NULL;
> +    trace_nbd_receive_simple_option_request(opt, nbd_opt_lookup(opt));

Naming of the trace doesn't quite match the naming of the function...

> +    if (nbd_send_option_request(ioc, opt, 0, NULL, errp) < 0) {
> +        return -1;
>      }
>  
> -    trace_nbd_receive_starttls_reply();
> -    if (nbd_receive_option_reply(ioc, NBD_OPT_STARTTLS, &reply, errp) < 0) {
> -        return NULL;
> +    trace_nbd_receive_simple_option_reply(opt, nbd_opt_lookup(opt));

[1] ...However, trace_nbd_request_simple_option_request sounds silly.
Can we get by with the shorter nbd_simple_option() as the function name,
then have two traces nbd_simple_option_request and
nbd_simple_option_reply?  Or, recognize that nbd_receive_option_reply()
already traces everything, so this trace is redundant with that one.  In
fact, nbd_send_option_request also traces everything (it doesn't trace
payload, but we aren't sending payload; the only time we need two traces
instead of one is if the local trace can document payload before the
common trace documents everything else.  Another reason for redundant
traces is if you envision needing to trace one local thing without being
overwhelmed by the noise of a common trace firing for unrelated events,
but NBD startup is not that frequent to need fine-grained filtering on
the traces).

> +    if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
> +        return -1;
>      }
>  
> -    if (reply.type != NBD_REP_ACK) {
> -        error_setg(errp, "Server rejected request to start TLS %" PRIx32,
> -                   reply.type);
> +    if (reply.length != 0) {
> +        error_setg(errp, "Option %d ('%s') response length is %" PRIu32
> +                   " (it should be zero)", opt, nbd_opt_lookup(opt),
> +                   reply.length);
>          nbd_send_opt_abort(ioc);

Just thinking aloud here. For starttls, the NBD spec already says
payload must be 0 length. But for NBD_OPT_STRUCTURED_REPLY, which is not
yet standardized, it is conceivable that in the future, we might have a
way for the server to return a payload to the ACK reply, which informs
the client what additional options have been unlocked by negotiating
structured reply (after all, we document that some options should not be
negotiated unless structured reply is negotiated first).  We don't have
that in the spec now, but writing our client to reject any payload from
the server (rather than silently ignoring the payload) means that we
can't ever write a server with that extended reply.  So it's worth
discussing on the NBD list whether we want to make any guarantees about
forcing a 0-length payload vs. allowing clients to ignore an
unrecognized non-empty payload.  Depending on what the NBD community
decides, the easiest approach here would be a 'bool ignore_payload'
parameter, true when requesting OPT_STRUCTURED_REPLY, false when
requesting OPT_STARTTLS.  But as this is all speculation, I'm also fine
with checking in your stricter version as-is for now; we have until qemu
2.11 is released to tweak our implementation as followups based on
conversations on the NBD list.

> +
> +    trace_nbd_receive_simple_option_approved(opt, nbd_opt_lookup(opt));

Another trace that doesn't add much value.

So, here's what I'm squashing, before I give:
Reviewed-by: Eric Blake <address@hidden>

diff --git i/nbd/client.c w/nbd/client.c
index c8702a80b1..fa64ec1c5b 100644
--- i/nbd/client.c
+++ w/nbd/client.c
@@ -540,7 +540,7 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
     }
 }

-/* nbd_request_simple_option
+/* nbd_request_simple_option: Send an option request, and parse the reply
  * return 1 for successful negotiation,
  *        0 if operation is unsupported,
  *        -1 with errp set for any other error
@@ -549,12 +549,10 @@ static int nbd_request_simple_option(QIOChannel
*ioc, int opt, Error **errp)
 {
     nbd_opt_reply reply;

-    trace_nbd_receive_simple_option_request(opt, nbd_opt_lookup(opt));
     if (nbd_send_option_request(ioc, opt, 0, NULL, errp) < 0) {
         return -1;
     }

-    trace_nbd_receive_simple_option_reply(opt, nbd_opt_lookup(opt));
     if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
         return -1;
     }
@@ -579,7 +577,6 @@ static int nbd_request_simple_option(QIOChannel
*ioc, int opt, Error **errp)
         return -1;
     }

-    trace_nbd_receive_simple_option_approved(opt, nbd_opt_lookup(opt));
     return 1;
 }

diff --git i/nbd/trace-events w/nbd/trace-events
index 0c8138ab92..59c0718a6b 100644
--- i/nbd/trace-events
+++ w/nbd/trace-events
@@ -9,9 +9,6 @@ nbd_opt_go_info_unknown(int info, const char *name)
"Ignoring unknown info %d (%
 nbd_opt_go_info_block_size(uint32_t minimum, uint32_t preferred,
uint32_t maximum) "Block sizes are 0x%" PRIx32 ", 0x%" PRIx32 ", 0x%" PRIx32
 nbd_receive_query_exports_start(const char *wantname) "Querying export
list for '%s'"
 nbd_receive_query_exports_success(const char *wantname) "Found desired
export name '%s'"
-nbd_receive_simple_option_request(int opt, const char *name)
"Requesting option %d (%s) from server"
-nbd_receive_simple_option_reply(int opt, const char *name) "Getting
reply for option %d (%s) from server"
-nbd_receive_simple_option_approved(int opt, const char *name) "Option
%d (%s) approved"
 nbd_receive_starttls_new_client(void) "Setting up TLS"
 nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
 nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving
negotiation tlscreds=%p hostname=%s"


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature