| Message ID | 1399964572-5376-8-git-send-email-marc.mari.barcelo@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 05/13/2014 01:02 AM, Marc Marí wrote: > Modify debug macros to have the same format through the codebase and use regular > ifs instead of ifdef. > > As the debug printf is always put in code, some casting had to be added to avoid > warnings treated as errors at compile time. Same comments as in 1/16 about long lines and gcc extensions (probably for the entire series). Additionally... > > Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com> > --- > hw/net/stellaris_enet.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c > index d04e6a4..f6737a9 100644 > --- a/hw/net/stellaris_enet.c > +++ b/hw/net/stellaris_enet.c > @@ -13,16 +13,17 @@ > //#define DEBUG_STELLARIS_ENET 1 > > #ifdef DEBUG_STELLARIS_ENET > -#define DPRINTF(fmt, ...) \ > -do { printf("stellaris_enet: " fmt , ## __VA_ARGS__); } while (0) In this case, the old code was also relying on the gcc extension. But pre-patch, the gcc extension was only encountered if DEBUG_StELLARIS_ENET was set (probably only by a developer using gcc for temporary debugging), while post-patch the gcc extension is ALWAYS encountered, regardless of developer. > -#define BADF(fmt, ...) \ > -do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__); exit(1);} while (0) In the old code, BADF() could be used in a single statement context, such as: if (foo) BADF("blah") else something(); (of course, that violates our coding style, but hear me out). > +#define DEBUG_STELLARIS_ENET_ENABLED 1 > #else > -#define DPRINTF(fmt, ...) do {} while(0) > -#define BADF(fmt, ...) \ > -do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);} while (0) > +#define DEBUG_STELLARIS_ENET_ENABLED 0 > #endif > > +#define DPRINTF(fmt, ...) QEMU_DPRINTF(DEBUG_STELLARIS_ENET_ENABLED, "stellaris_enet", fmt, ## __VA_ARGS__) > + > +#define BADF(fmt, ...) \ > + QEMU_DPRINTF(1, "stellaris_enet error", fmt, ## __VA_ARGS__); \ > + do { if (DEBUG_STELLARIS_ENET_ENABLED) { exit(1); } } while (0) However, in the new code, BADF() is no longer a single statement. You either need to move the "do {" to appear before the QEMU_PRINTF [preferred], or you can just declare that this is no longer a single-statement macro at which point you could just drop the do/while altogether [depends on our coding standard for safety]. The example above, even though it violates coding standards, should demonstrate why your change is wrong - the bare "else" is now a syntax error. > > - DPRINTF("Received packet len=%d\n", size); > + DPRINTF("Received packet len=%d\n", (int)size); Ah, we FINALLY get to an added cast. What you SHOULD be doing is using len=%zu coupled with un-casted size, rather than papering over the type mismatch. > n = s->next_packet + s->np; > if (n >= 31) > n -= 31; > @@ -212,14 +213,14 @@ static void stellaris_enet_write(void *opaque, hwaddr offset, > switch (offset) { > case 0x00: /* IACK */ > s->ris &= ~value; > - DPRINTF("IRQ ack %02x/%02x\n", value, s->ris); > + DPRINTF("IRQ ack %02x/%02x\n", (unsigned)value, s->ris); > stellaris_enet_update(s); > /* Clearing TXER also resets the TX fifo. */ > if (value & SE_INT_TXER) > s->tx_frame_len = -1; > break; > case 0x04: /* IM */ > - DPRINTF("IRQ mask %02x/%02x\n", value, s->ris); > + DPRINTF("IRQ mask %02x/%02x\n", (unsigned)value, s->ris); More cases where you should be fixing the % format to use the correct type, rather than adding a cast.
diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c index d04e6a4..f6737a9 100644 --- a/hw/net/stellaris_enet.c +++ b/hw/net/stellaris_enet.c @@ -13,16 +13,17 @@ //#define DEBUG_STELLARIS_ENET 1 #ifdef DEBUG_STELLARIS_ENET -#define DPRINTF(fmt, ...) \ -do { printf("stellaris_enet: " fmt , ## __VA_ARGS__); } while (0) -#define BADF(fmt, ...) \ -do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__); exit(1);} while (0) +#define DEBUG_STELLARIS_ENET_ENABLED 1 #else -#define DPRINTF(fmt, ...) do {} while(0) -#define BADF(fmt, ...) \ -do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);} while (0) +#define DEBUG_STELLARIS_ENET_ENABLED 0 #endif +#define DPRINTF(fmt, ...) QEMU_DPRINTF(DEBUG_STELLARIS_ENET_ENABLED, "stellaris_enet", fmt, ## __VA_ARGS__) + +#define BADF(fmt, ...) \ + QEMU_DPRINTF(1, "stellaris_enet error", fmt, ## __VA_ARGS__); \ + do { if (DEBUG_STELLARIS_ENET_ENABLED) { exit(1); } } while (0) + #define SE_INT_RX 0x01 #define SE_INT_TXER 0x02 #define SE_INT_TXEMP 0x04 @@ -97,7 +98,7 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si return -1; } - DPRINTF("Received packet len=%d\n", size); + DPRINTF("Received packet len=%d\n", (int)size); n = s->next_packet + s->np; if (n >= 31) n -= 31; @@ -212,14 +213,14 @@ static void stellaris_enet_write(void *opaque, hwaddr offset, switch (offset) { case 0x00: /* IACK */ s->ris &= ~value; - DPRINTF("IRQ ack %02x/%02x\n", value, s->ris); + DPRINTF("IRQ ack %02x/%02x\n", (unsigned)value, s->ris); stellaris_enet_update(s); /* Clearing TXER also resets the TX fifo. */ if (value & SE_INT_TXER) s->tx_frame_len = -1; break; case 0x04: /* IM */ - DPRINTF("IRQ mask %02x/%02x\n", value, s->ris); + DPRINTF("IRQ mask %02x/%02x\n", (unsigned)value, s->ris); s->im = value; stellaris_enet_update(s); break;
Modify debug macros to have the same format through the codebase and use regular ifs instead of ifdef. As the debug printf is always put in code, some casting had to be added to avoid warnings treated as errors at compile time. Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com> --- hw/net/stellaris_enet.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)