Re: [Qemu-devel] [PATCH 10/14] nbd/client: Split handshake into two functions

[Vladimir Sementsov-Ogievskiy]

01.12.2018 1:03, 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).
> 
> Signed-off-by: Eric Blake <address@hidden>

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   nbd/client.c     | 142 ++++++++++++++++++++++++++++++-----------------
>   nbd/trace-events |   2 +-
>   2 files changed, 92 insertions(+), 52 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 1ed5009642e..a282712724d 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -768,21 +768,22 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>       return received || opt == NBD_OPT_LIST_META_CONTEXT;
>   }
> 
> -int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> -                          const char *hostname, QIOChannel **outioc,
> -                          NBDExportInfo *info, Error **errp)
> +/* Start the handshake to the server.  After a positive return, the server
> + * is ready to accept additional NBD_OPT requests.
> + * 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
> + */

hmm, may be, introduce enum of named constants, instead of raw numbers?

NBD_STARTED_OLD_STYLE
NBD_STARTED_NEW_STYLE
NBD_STARTED_NEW_STYLE_FIXED
NBD_STARTED_STRUCTURED_REPLIES

or, at least, add short comment after each return statement, to not scroll up
every time (like you gracefully do after each case: statement).

However, I'm okay with either way.


-- 
Best regards,
Vladimir