Re: [Qemu-devel] [PATCH 5/9] nbd/client: fix error messages in nbd_handle_reply_err

[Vladimir Sementsov-Ogievskiy]

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.

--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 5/9] nbd/client: fix error messages in nbd_handle_reply_err

[Eric Blake]

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.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH 5/9] nbd/client: fix error messages in nbd_handle_reply_err

[Vladimir Sementsov-Ogievskiy]

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 
---
 nbd/client.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

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;
 }
-- 
2.11.1