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

[Vladimir Sementsov-Ogievskiy]

07.02.2017 19:32, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> >   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

Hmm.. The caller should free it in this case. Currently, the 
get_opt_name is called only on error path or if tracing enabled. How to 
achieve this with variable string return value not complicating the code 
a lot? When there is unknown message there should be other mechanism to 
inform user

> 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.


--
Best regards,
Vladimir