[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 04/18] nbd/client: refactor nbd_receive_starttls
Date: Thu, 9 Feb 2017 09:20:13 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1

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

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,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]