qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH 4/9] ATAPI: Replace DEBUG_IDE_ATAPI


From: John Snow
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH 4/9] ATAPI: Replace DEBUG_IDE_ATAPI with tracing events
Date: Mon, 28 Aug 2017 19:43:13 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1


On 08/08/2017 04:59 PM, Eric Blake wrote:
> On 08/08/2017 01:33 PM, John Snow wrote:
>> Goodbye, printfs.
>> Hello, fancy printfs.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  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?
> 

Probably.

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

I'll fix these and let you re-bestow me with RBs, to err on the side of
caution.

> Reviewed-by: Eric Blake <address@hidden>
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]