| Message ID | 20181130220344.3350618-3-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
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 <eblake@redhat.com> > --- > 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 <rjones@redhat.com> Rich.
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 <eblake@redhat.com> > --- > 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 <vsementsov@virtuozzo.com> > + 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; > } >
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 <eblake@redhat.com> >> --- >> + 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 <vsementsov@virtuozzo.com> That's minor enough; I'll add commas to all instances, but keep your R-b.
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; }
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 <eblake@redhat.com> --- nbd/client.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)