On 07/07/2017 10:29 AM, Vladimir Sementsov-Ogievskiy wrote: > Let NBD use the trace mechanisms already present in qemu. Now you can > use the -trace optino of qemu, or the -T/--trace option of qemu-img,
s/optino/option/
> qemu-io, and qemu-nbd, to select nbd traces. For qemu, the QMP commands
> trace-event-{get,set}-state can also toggle tracing on the fly.
>
> Example:
> qemu-nbd --trace 'nbd_*' <image file> # enables all nbd traces
>
> Recompilation with CFLAGS=-DDEBUG_NBD is no more needed, furthermore,
> DEBUG_NBD macro is removed from the code.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> Makefile.objs | 1 +
> nbd/client.c | 70
> ++++++++++++++++++++++++------------------------------
> nbd/nbd-internal.h | 19 ---------------
> nbd/server.c | 67 ++++++++++++++++++++++++---------------------------
> nbd/trace-events | 58 ++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 121 insertions(+), 94 deletions(-)
> create mode 100644 nbd/trace-events
We may still come up with further tweaks, but I have a series that I
plan on posting on top of this, so I'm fine if it goes in as-is (with
the commit message typo fixed).
Reviewed-by: Eric Blake <ebl...@redhat.com>
> @@ -342,12 +341,12 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
> {
> bool foundExport = false;
>
> - TRACE("Querying export list for '%s'", wantname);
> + trace_nbd_receive_query_exports_start(wantname);
> if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
> return -1;
> }
>
> - TRACE("Reading available export names");
> + trace_nbd_receive_query_exports_loop();
Not sure this one adds much useful information.
> @@ -462,7 +460,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char
> *name, uint16_t *flags,
> }
>
> magic = ldq_be_p(buf);
> - TRACE("Magic is 0x%" PRIx64, magic);
> + trace_nbd_receive_negotiate_magic(magic);
>
> if (memcmp(buf, "NBDMAGIC", 8) != 0) {
> error_setg(errp, "Invalid magic received");
> @@ -474,7 +472,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char
> *name, uint16_t *flags,
> goto fail;
> }
> magic = be64_to_cpu(magic);
> - TRACE("Magic is 0x%" PRIx64, magic);
> + trace_nbd_receive_negotiate_magic2(magic);
I still think you only need one trace function, called twice (rather
than two separate names with identical implementation)
> @@ -396,15 +396,15 @@ static int nbd_negotiate_options(NBDClient *client,
> Error **errp)
> error_prepend(errp, "read failed: ");
> return -EIO;
> }
> - TRACE("Checking client flags");
> + trace_nbd_negotiate_options_flags();
> be32_to_cpus(&flags);
> if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
> - TRACE("Client supports fixed newstyle handshake");
> + trace_nbd_negotiate_options_newstyle();
> fixedNewstyle = true;
> flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
> }
> if (flags & NBD_FLAG_C_NO_ZEROES) {
> - TRACE("Client supports no zeroes at handshake end");
> + trace_nbd_negotiate_options_no_zeroes();
My followup series will merge these three into one trace (print the
value once, rather than one trace per bit)
> @@ -422,7 +422,7 @@ static int nbd_negotiate_options(NBDClient *client, Error
> **errp)
> error_prepend(errp, "read failed: ");
> return -EINVAL;
> }
> - TRACE("Checking opts magic");
> + trace_nbd_negotiate_options_check_magic();
Tracing the magic received might be useful.
> @@ -501,8 +501,8 @@ static int nbd_negotiate_options(NBDClient *client, Error
> **errp)
> &local_err);
>
> if (local_err != NULL) {
> - TRACE("Reply to NBD_OPT_ABORT request failed: %s",
> - error_get_pretty(local_err));
> + const char *error = error_get_pretty(local_err);
> + trace_nbd_opt_abort_reply_failed(error);
> error_free(local_err);
> }
Overkill. Who cares if sending the reply to NBD_OPT_ABORT fails. I'll
clean that up in my followup series.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
