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 <[email protected]> >> --- >> 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 <[email protected]> >
