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.barc...@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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
