diff mbox series

[5/9] nbd/client: fix error messages in nbd_handle_reply_err

Message ID 1518702707-7077-6-git-send-email-vsementsov@virtuozzo.com
State New
Headers show
Series [1/9] nbd/server: add nbd_opt_invalid helper | expand

Commit Message

Vladimir Sementsov-Ogievskiy Feb. 15, 2018, 1:51 p.m. UTC 
1. NBD_REP_ERR_INVALID is not only about length, so, make message more
   general

2. hex format is not very good: it's hard to read something like
   "option a (set meta context)", so switch to dec.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Eric Blake Feb. 16, 2018, 5:38 p.m. UTC | #1 
On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> 1. NBD_REP_ERR_INVALID is not only about length, so, make message more
>     general
> 
> 2. hex format is not very good: it's hard to read something like
>     "option a (set meta context)", so switch to dec.

It would be okay as option 0xa; I also want to be sure we match the 
output in server traces with the output in client traces; for example, I 
found:

nbd/server.c-                error_setg(errp, "Unsupported option 0x%" 
PRIx32 " (%s)",
nbd/server.c:                           option, nbd_opt_lookup(option));

So we want consistency through all the call sites, before this patch is 
ready to go, although I agree we need it.
Vladimir Sementsov-Ogievskiy March 1, 2018, 11:38 a.m. UTC | #2 
16.02.2018 20:38, Eric Blake wrote:
> On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 1. NBD_REP_ERR_INVALID is not only about length, so, make message more
>>     general
>>
>> 2. hex format is not very good: it's hard to read something like
>>     "option a (set meta context)", so switch to dec.
>
> It would be okay as option 0xa; I also want to be sure we match the 
> output in server traces with the output in client traces; for example, 
> I found:
>
> nbd/server.c-                error_setg(errp, "Unsupported option 0x%" 
> PRIx32 " (%s)",
> nbd/server.c:                           option, nbd_opt_lookup(option));
>
> So we want consistency through all the call sites, before this patch 
> is ready to go, although I agree we need it.
>

I think, decimal format for NBD constants is better, as it used in the spec.
diff mbox series

Patch

diff --git a/nbd/client.c b/nbd/client.c
index 89f80f9590..1f730341c0 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -180,22 +180,22 @@  static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
         goto cleanup;
 
     case NBD_REP_ERR_POLICY:
-        error_setg(errp, "Denied by server for option %" PRIx32 " (%s)",
+        error_setg(errp, "Denied by server for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_INVALID:
-        error_setg(errp, "Invalid data length for option %" PRIx32 " (%s)",
+        error_setg(errp, "Invalid parameters for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_PLATFORM:
-        error_setg(errp, "Server lacks support for option %" PRIx32 " (%s)",
+        error_setg(errp, "Server lacks support for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_TLS_REQD:
-        error_setg(errp, "TLS negotiation required before option %" PRIx32
+        error_setg(errp, "TLS negotiation required before option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
 
@@ -204,17 +204,17 @@  static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
         break;
 
     case NBD_REP_ERR_SHUTDOWN:
-        error_setg(errp, "Server shutting down before option %" PRIx32 " (%s)",
+        error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_BLOCK_SIZE_REQD:
-        error_setg(errp, "Server requires INFO_BLOCK_SIZE for option %" PRIx32
+        error_setg(errp, "Server requires INFO_BLOCK_SIZE for option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
 
     default:
-        error_setg(errp, "Unknown error code when asking for option %" PRIx32
+        error_setg(errp, "Unknown error code when asking for option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
     }