Re: [Qemu-devel] [PATCH 04/18] nbd/client: refactor nbd_receive_starttls

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Split out nbd_receive_simple_option to be reused for structured reply
> option.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client.c       | 54 +++++++++++++++++++++++++++++++++++-------------------
>  nbd/nbd-internal.h | 14 ++++++++++++++
>  2 files changed, 49 insertions(+), 19 deletions(-)
> 

> +++ b/nbd/nbd-internal.h
> @@ -96,6 +96,20 @@
>  #define NBD_ENOSPC     28
>  #define NBD_ESHUTDOWN  108
>  
> +static inline const char *nbd_opt_name(int opt)
> +{
> +    switch (opt) {
> +    case NBD_OPT_EXPORT_NAME: return "export_name";

Does this really get past checkpatch?

> +    case NBD_OPT_ABORT: return "abort";
> +    case NBD_OPT_LIST: return "list";
> +    case NBD_OPT_PEEK_EXPORT: return "peek_export";
> +    case NBD_OPT_STARTTLS: return "tls";

Why just 'tls' instead of 'starttls'?

> +    case NBD_OPT_STRUCTURED_REPLY: return "structured_reply";
> +    }
> +
> +    return "<unknown option>";

Can you please consider making this include the %d representation of the
unknown option; perhaps by snprintf'ing into static storage?  While it
is unlikely that a well-behaved server will respond to a client with an
option the client doesn't recognize, it is much more likely that this
reverse lookup function will be used in a server to respond to an
unknown option from a client.

In fact, I might have split this into two patches: one providing
nbd_opt_name() and using it throughout the code base where appropriate,
and the other refactoring starttls in the client.

I'm not sure if the reverse lookup function needs to be inline in the
header; it could reasonably live in nbd/common.c, particularly if you
are going to take my advice to have it format a message for unknown values.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]