Re: [Qemu-devel] [PATCH v3 10/10] nbd: use generic trace subsystem instead of TRACE macro

[Eric Blake]

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 <address@hidden>
> ---
>  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 <address@hidden>

> @@ -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