Re: [Qemu-devel] [PATCH v2 07/16] stellaris: Convert conditional compilation of debug printfs to regular ifs

[Eric Blake <eblake@redhat.com>]

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

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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