Re: [Qemu-devel] [PATCH 4/9] ATAPI: Replace DEBUG_IDE_ATAPI with tracing events

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

On 08/08/2017 01:33 PM, John Snow wrote:
> Goodbye, printfs.
> Hello, fancy printfs.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/atapi.c            | 64 +++++++++++++++++------------------------------
>  hw/ide/trace-events       | 19 ++++++++++++++
>  include/hw/ide/internal.h |  1 -
>  3 files changed, 42 insertions(+), 42 deletions(-)

> @@ -1330,16 +1310,18 @@ void ide_atapi_cmd(IDEState *s)
>      uint8_t *buf = s->io_buffer;
>      const struct AtapiCmd *cmd = &atapi_cmd_table[s->io_buffer[0]];
>  
> -#ifdef DEBUG_IDE_ATAPI
> -    {
> +    trace_ide_atapi_cmd(s, s->io_buffer[0]);
> +
> +    if (TRACE_IDE_ATAPI_CMD_PACKET_ENABLED) {
> +        /* Each pretty-printed byte needs two bytes and a space; */
> +        char *ppacket = g_malloc(ATAPI_PACKET_SIZE * 3 + 1);

Nice use of an extra conditional to make a pretty trace without the
overhead when tracing is off.  (Oh yeah, you asked me on IRC how to do
it, and I pointed to commit bd6952a3 as the example)


> +++ b/hw/ide/trace-events
> @@ -6,6 +6,10 @@ ide_ioport_read(uint32_t addr, const char *reg, uint32_t val, void *bus, void *s
>  ide_ioport_write(uint32_t addr, const char *reg, uint32_t val, void *bus, void *s) "IDE PIO wr @ 0x%"PRIx32" (%s); val 0x%02"PRIx32"; bus %p IDEState %p"

Wouldn't it be nice if our trace generator would let us line-wrap?

> +# Warning: Verbose

Cute.  Should you also add it to the ide_data_* traces, per the commit
message in 3/9?

> +ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: %p; limit=0x%x packet: %s"
> \ No newline at end of file

Umm, that should be fixed (I know that 'diff' uses both / and \ to
differentiate whether it was the pre- or post-patch version that had the
issue, but never remember which is which - so the fix may be in an
earlier patch).  Also fix the trace-events rebase for 3/9; with that,
you can add

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]