Re: [Qemu-devel] [PATCH v3 13/19] nbd/client: Split handshake into two functions

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

12.01.2019 20:58, Eric Blake wrote:
> An upcoming patch will add the ability for qemu-nbd to list
> the services provided by an NBD server.  Share the common
> code of the TLS handshake by splitting the initial exchange
> into a separate function, leaving only the export handling
> in the original function.  Functionally, there should be no
> change in behavior in this patch, although some of the code
> motion may be difficult to follow due to indentation changes
> (view with 'git diff -w' for a smaller changeset).
> 
> I considered an enum for the return code coordinating state
> between the two functions, but in the end just settled with
> ample comments.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Message-Id: <20181215135324.152629-17-eblake@redhat.com>
> ---
>   nbd/client.c     | 144 +++++++++++++++++++++++++++++++----------------
>   nbd/trace-events |   2 +-
>   2 files changed, 95 insertions(+), 51 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index fa931fd8e5d..5053433ea5e 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -813,21 +813,24 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
>       return received;
>   }
> 
> -int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> -                          const char *hostname, QIOChannel **outioc,
> -                          NBDExportInfo *info, Error **errp)
> +/*
> + * nbd_start_negotiate:
> + * Start the handshake to the server.  After a positive return, the server
> + * is ready to accept additional NBD_OPT requests.

+ * @zeroes must be set to true before call !!!


> + * Returns: negative errno: failure talking to server
> + *          0: server is oldstyle, client must still parse export size
> + *          1: server is newstyle, but can only accept EXPORT_NAME
> + *          2: server is newstyle, but lacks structured replies
> + *          3: server is newstyle and set up for structured replies
> + */
> +static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> +                               const char *hostname, QIOChannel **outioc,
> +                               bool structured_reply, bool *zeroes,
> +                               Error **errp)
>   {
>       uint64_t magic;
> -    bool zeroes = true;
> -    bool structured_reply = info->structured_reply;
> -    bool base_allocation = info->base_allocation;
> 
> -    trace_nbd_receive_negotiate(tlscreds, hostname ? hostname : "<null>");
> -
> -    assert(info->name);
> -    trace_nbd_receive_negotiate_name(info->name);
> -    info->structured_reply = false;
> -    info->base_allocation = false;
> +    trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");

or, a lot better:

+    *zeroes = true


> 
>       if (outioc) {
>           *outioc = NULL;
> @@ -872,7 +875,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>               clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
>           }
>           if (globalflags & NBD_FLAG_NO_ZEROES) {
> -            zeroes = false;
> +            *zeroes = false;
>               clientflags |= NBD_FLAG_C_NO_ZEROES;
>           }
>           /* client requested flags */

[..]

> +/*
> + * nbd_receive_negotiate:
> + * Connect to server, complete negotiation, and move into transmission phase.
> + * Returns: negative errno: failure talking to server
> + *          0: server is connected
> + */
> +int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> +                          const char *hostname, QIOChannel **outioc,
> +                          NBDExportInfo *info, Error **errp)
> +{
> +    int result;
> +    bool zeroes = true;

and then, this initialization may be dropped (or left as is, if gcc like it)

> +    bool base_allocation = info->base_allocation;
> +    uint32_t oldflags;
> +
> +    assert(info->name);
> +    trace_nbd_receive_negotiate_name(info->name);
> +
> +    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
> +                                 info->structured_reply, &zeroes, errp);
> +



-- 
Best regards,
Vladimir