Re: [Qemu-devel] [PATCH 02/14] nbd/client: More consistent error messages

2018-12-10 Thread Eric Blake 

On 12/5/18 9:03 AM, Vladimir Sementsov-Ogievskiy wrote:

01.12.2018 1:03, Eric Blake wrote:

Consolidate on using decimal (not hex) and on outputting the
option reply name (not just value) when the client reports
protocol discrepancies from the server.  While it won't affect
normal operation, it makes debugging additions easier.

Signed-off-by: Eric Blake 
---



+error_setg(errp, "Unexpected option type %u (%s) expected %u (%s)",
+   reply->option, nbd_opt_lookup(reply->option),
+   opt, nbd_opt_lookup(opt));



@@ -378,9 +380,9 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
   return 1;
   }
   if (reply.type != NBD_REP_INFO) {
-error_setg(errp, "unexpected reply type %" PRIu32
-   " (%s), expected %u",
-   reply.type, nbd_rep_lookup(reply.type), NBD_REP_INFO);
+error_setg(errp, "unexpected reply type %u (%s), expected %u (%s)",


hmm, we are definitely inconsistent about having comma before "expected" word...

anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy 


That's minor enough; I'll add commas to all instances, but keep your R-b.

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

Re: [Qemu-devel] [PATCH 02/14] nbd/client: More consistent error messages

2018-12-05 Thread Vladimir Sementsov-Ogievskiy 
01.12.2018 1:03, Eric Blake wrote:
> Consolidate on using decimal (not hex) and on outputting the
> option reply name (not just value) when the client reports
> protocol discrepancies from the server.  While it won't affect
> normal operation, it makes debugging additions easier.
> 
> Signed-off-by: Eric Blake 
> ---
>   nbd/client.c | 21 -
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index b4d457a19ad..b667a1b56fd 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -132,8 +132,9 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
> uint32_t opt,
>   return -1;
>   }
>   if (reply->option != opt) {
> -error_setg(errp, "Unexpected option type %x expected %x",
> -   reply->option, opt);
> +error_setg(errp, "Unexpected option type %u (%s) expected %u (%s)",
> +   reply->option, nbd_opt_lookup(reply->option),
> +   opt, nbd_opt_lookup(opt));
>   nbd_send_opt_abort(ioc);
>   return -1;
>   }
> @@ -265,8 +266,9 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, bool *match,
>   }
>   return 0;
>   } else if (reply.type != NBD_REP_SERVER) {
> -error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
> -   reply.type, NBD_REP_SERVER);
> +error_setg(errp, "Unexpected reply type %u (%s) expected %u (%s)",
> +   reply.type, nbd_rep_lookup(reply.type),
> +   NBD_REP_SERVER, nbd_rep_lookup(NBD_REP_SERVER));
>   nbd_send_opt_abort(ioc);
>   return -1;
>   }
> @@ -378,9 +380,9 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
> *wantname,
>   return 1;
>   }
>   if (reply.type != NBD_REP_INFO) {
> -error_setg(errp, "unexpected reply type %" PRIu32
> -   " (%s), expected %u",
> -   reply.type, nbd_rep_lookup(reply.type), NBD_REP_INFO);
> +error_setg(errp, "unexpected reply type %u (%s), expected %u 
> (%s)",

hmm, we are definitely inconsistent about having comma before "expected" word...

anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy 

> +   reply.type, nbd_rep_lookup(reply.type),
> +   NBD_REP_INFO, nbd_rep_lookup(NBD_REP_INFO));
>   nbd_send_opt_abort(ioc);
>   return -1;
>   }
> @@ -704,8 +706,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>   }
> 
>   if (reply.type != NBD_REP_ACK) {
> -error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
> -   reply.type, NBD_REP_ACK);
> +error_setg(errp, "Unexpected reply type %u (%s) expected %u (%s)",
> +   reply.type, nbd_rep_lookup(reply.type),
> +   NBD_REP_ACK, nbd_rep_lookup(NBD_REP_ACK));
>   nbd_send_opt_abort(ioc);
>   return -1;
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 02/14] nbd/client: More consistent error messages

2018-11-30 Thread Richard W.M. Jones 
On Fri, Nov 30, 2018 at 04:03:31PM -0600, Eric Blake wrote:
> Consolidate on using decimal (not hex) and on outputting the
> option reply name (not just value) when the client reports
> protocol discrepancies from the server.  While it won't affect
> normal operation, it makes debugging additions easier.
> 
> Signed-off-by: Eric Blake 
> ---
>  nbd/client.c | 21 -
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index b4d457a19ad..b667a1b56fd 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -132,8 +132,9 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
> uint32_t opt,
>  return -1;
>  }
>  if (reply->option != opt) {
> -error_setg(errp, "Unexpected option type %x expected %x",
> -   reply->option, opt);
> +error_setg(errp, "Unexpected option type %u (%s) expected %u (%s)",
> +   reply->option, nbd_opt_lookup(reply->option),
> +   opt, nbd_opt_lookup(opt));
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
> @@ -265,8 +266,9 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
> *want, bool *match,
>  }
>  return 0;
>  } else if (reply.type != NBD_REP_SERVER) {
> -error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
> -   reply.type, NBD_REP_SERVER);
> +error_setg(errp, "Unexpected reply type %u (%s) expected %u (%s)",
> +   reply.type, nbd_rep_lookup(reply.type),
> +   NBD_REP_SERVER, nbd_rep_lookup(NBD_REP_SERVER));
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
> @@ -378,9 +380,9 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
> *wantname,
>  return 1;
>  }
>  if (reply.type != NBD_REP_INFO) {
> -error_setg(errp, "unexpected reply type %" PRIu32
> -   " (%s), expected %u",
> -   reply.type, nbd_rep_lookup(reply.type), NBD_REP_INFO);
> +error_setg(errp, "unexpected reply type %u (%s), expected %u 
> (%s)",
> +   reply.type, nbd_rep_lookup(reply.type),
> +   NBD_REP_INFO, nbd_rep_lookup(NBD_REP_INFO));
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }
> @@ -704,8 +706,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>  }
> 
>  if (reply.type != NBD_REP_ACK) {
> -error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
> -   reply.type, NBD_REP_ACK);
> +error_setg(errp, "Unexpected reply type %u (%s) expected %u (%s)",
> +   reply.type, nbd_rep_lookup(reply.type),
> +   NBD_REP_ACK, nbd_rep_lookup(NBD_REP_ACK));
>  nbd_send_opt_abort(ioc);
>  return -1;
>  }

The NBD protocol doc seems to use integers pretty consistently (and
certainly not "bare" hex numbers).  Obviously having the mnemonic name
too is helpful.  So:

Reviewed-by: Richard W.M. Jones 

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

[Qemu-devel] [PATCH 02/14] nbd/client: More consistent error messages

2018-11-30 Thread Eric Blake 
Consolidate on using decimal (not hex) and on outputting the
option reply name (not just value) when the client reports
protocol discrepancies from the server.  While it won't affect
normal operation, it makes debugging additions easier.

Signed-off-by: Eric Blake 
---
 nbd/client.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index b4d457a19ad..b667a1b56fd 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -132,8 +132,9 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
uint32_t opt,
 return -1;
 }
 if (reply->option != opt) {
-error_setg(errp, "Unexpected option type %x expected %x",
-   reply->option, opt);
+error_setg(errp, "Unexpected option type %u (%s) expected %u (%s)",
+   reply->option, nbd_opt_lookup(reply->option),
+   opt, nbd_opt_lookup(opt));
 nbd_send_opt_abort(ioc);
 return -1;
 }
@@ -265,8 +266,9 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
*want, bool *match,
 }
 return 0;
 } else if (reply.type != NBD_REP_SERVER) {
-error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
-   reply.type, NBD_REP_SERVER);
+error_setg(errp, "Unexpected reply type %u (%s) expected %u (%s)",
+   reply.type, nbd_rep_lookup(reply.type),
+   NBD_REP_SERVER, nbd_rep_lookup(NBD_REP_SERVER));
 nbd_send_opt_abort(ioc);
 return -1;
 }
@@ -378,9 +380,9 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
 return 1;
 }
 if (reply.type != NBD_REP_INFO) {
-error_setg(errp, "unexpected reply type %" PRIu32
-   " (%s), expected %u",
-   reply.type, nbd_rep_lookup(reply.type), NBD_REP_INFO);
+error_setg(errp, "unexpected reply type %u (%s), expected %u (%s)",
+   reply.type, nbd_rep_lookup(reply.type),
+   NBD_REP_INFO, nbd_rep_lookup(NBD_REP_INFO));
 nbd_send_opt_abort(ioc);
 return -1;
 }
@@ -704,8 +706,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
*ioc,
 }

 if (reply.type != NBD_REP_ACK) {
-error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
-   reply.type, NBD_REP_ACK);
+error_setg(errp, "Unexpected reply type %u (%s) expected %u (%s)",
+   reply.type, nbd_rep_lookup(reply.type),
+   NBD_REP_ACK, nbd_rep_lookup(NBD_REP_ACK));
 nbd_send_opt_abort(ioc);
 return -1;
 }
-- 
2.17.2