[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 17/43] net: made printf always compile in debug
|
From: |
Thomas Huth |
|
Subject: |
Re: [Qemu-devel] [PATCH 17/43] net: made printf always compile in debug output |
|
Date: |
Mon, 3 Apr 2017 11:40:39 +0200 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
Hi,
thanks for looking into this issue ... but I've got some comments:
On 01.04.2017 15:52, Danil Antonov wrote:
> From d01cd76d0b20ee8fa67c07da64b0e2301e510247 Mon Sep 17 00:00:00 2001
> From: Danil Antonov <address@hidden <mailto:address@hidden>>
> Date: Wed, 29 Mar 2017 12:30:42 +0300
> Subject: [PATCH 17/43] net: made printf always compile in debug output
These header lines should not be in the body of the mail ... looks like
something went wrong with your mail setup?
> Wrapped printf calls inside debug macros (DPRINTF) in `if` statement.
> This will ensure that printf function will always compile even if debug
> output is turned off and, in turn, will prevent bitrot of the format
> strings.
>
> Signed-off-by: Danil Antonov <address@hidden
> <mailto:address@hidden>>
... and please do not send HTML mails to the list. I strongly recommend
to set up a proper "git send-email" environment on your system if you
want to contribute more than one or two patches (which you obviously do
with your patch series of 43 patches already). And yes, you can use "git
send-email" with gmail, too!
> ---
> hw/net/dp8393x.c | 15 ++++++++++-----
> hw/net/lan9118.c | 20 ++++++++++++++------
> hw/net/mcf_fec.c | 18 +++++++++++-------
> hw/net/stellaris_enet.c | 28 ++++++++++++++++++----------
> 4 files changed, 53 insertions(+), 28 deletions(-)
[...]
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index 3db8937..427160a 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -22,17 +22,25 @@
>
> //#define DEBUG_LAN9118
I suggest you remove the above line now that you introduce the new one
below.
> +#ifndef DEBUG_LAN9118
> +#define DEBUG_LAN9118 0
> +#endif
> +
> #ifdef DEBUG_LAN9118
> -#define DPRINTF(fmt, ...) \
> -do { printf("lan9118: " fmt , ## __VA_ARGS__); } while (0)
> #define BADF(fmt, ...) \
> do { hw_error("lan9118: error: " fmt , ## __VA_ARGS__);} while (0)
> #else
> -#define DPRINTF(fmt, ...) do {} while(0)
> #define BADF(fmt, ...) \
> do { fprintf(stderr, "lan9118: error: " fmt , ## __VA_ARGS__);} while (0)
> #endif
Since DEBUG_LAN9118 is now always defined, BADF will always be defined
here, too - so this is a change in behavior. I suggest you change the
above "#ifdef DEBUG_LAN9118" into "#if DEBUG_LAN9118" instead.
> +#define DPRINTF(fmt, ...) \
> + do { \
> + if (DEBUG_LAN9118) { \
> + fprintf(stderr, "lan9118: " fmt, ## __VA_ARGS__); \
> + } \
> + } while (0)
> +
> #define CSR_ID_REV 0x50
> #define CSR_IRQ_CFG 0x54
> #define CSR_INT_STS 0x58
> @@ -1033,7 +1041,7 @@ static void lan9118_writel(void *opaque, hwaddr
> offset,
> s->int_sts |= val & SW_INT;
> break;
> case CSR_FIFO_INT:
> - DPRINTF("FIFO INT levels %08x\n", val);
> + DPRINTF("FIFO INT levels %08x\n", (int)val);
> s->fifo_int = val;
> break;
> case CSR_RX_CFG:
> @@ -1114,9 +1122,9 @@ static void lan9118_writel(void *opaque, hwaddr
> offset,
> if (val & 0x80000000) {
> if (val & 0x40000000) {
> s->mac_data = do_mac_read(s, val & 0xf);
> - DPRINTF("MAC read %d = 0x%08x\n", val & 0xf, s->mac_data);
> + DPRINTF("MAC read %lx = 0x%08x\n", val & 0xf, s->mac_data);
The correct way to print an uint64_t value is to use PRIx64 ...
otherwise you'll run into problems when compiling this code on a 64-bit
host.
> } else {
> - DPRINTF("MAC write %d = 0x%08x\n", val & 0xf, s->mac_data);
> + DPRINTF("MAC write %lx = 0x%08x\n", val & 0xf,
> s->mac_data);
PRIx64 again
> do_mac_write(s, val & 0xf, s->mac_data);
> }
> }
> diff --git a/hw/net/mcf_fec.c b/hw/net/mcf_fec.c
> index bfa6b4b..b1430ee 100644
> --- a/hw/net/mcf_fec.c
> +++ b/hw/net/mcf_fec.c
> @@ -18,12 +18,16 @@
>
> //#define DEBUG_FEC 1
Remove above line.
> -#ifdef DEBUG_FEC
> -#define DPRINTF(fmt, ...) \
> -do { printf("mcf_fec: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do {} while(0)
> -#endif
> +#ifndef DEBUG_FEC
> +#define DEBUG_FEC 0
> +#endif
> +
> +#define DPRINTF(fmt, ...) \
> + do { \
> + if (DEBUG_FEC) { \
> + fprintf(stderr, "mcf_fec: " fmt, ## __VA_ARGS__); \
> + } \
> + } while (0)
>
> #define FEC_MAX_DESC 1024
> #define FEC_MAX_FRAME_SIZE 2032
> @@ -557,7 +561,7 @@ static ssize_t mcf_fec_receive(NetClientState *nc,
> const uint8_t *buf, size_t si
> unsigned int buf_len;
> size_t retsize;
>
> - DPRINTF("do_rx len %d\n", size);
> + DPRINTF("do_rx len %lx\n", size);
I think this is not portable, too. Either use %zd (or %zu), or cast the
size parameter instead.
> if (!s->rx_enabled) {
> return -1;
> }
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 04bd10a..e6f5e28 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -13,16 +13,24 @@
>
> //#define DEBUG_STELLARIS_ENET 1
Remove above line
> -#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)
> -#else
> -#define DPRINTF(fmt, ...) do {} while(0)
> -#define BADF(fmt, ...) \
> -do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);}
> while (0)
> -#endif
> +#ifndef DEBUG_STELLARIS_ENET
> +#define DEBUG_STELLARIS_ENET 0
> +#endif
> +
> +#define DPRINTF(fmt, ...) \
> + do { \
> + if (DEBUG_STELLARIS_ENET) { \
> + fprintf(stderr, "stellaris_enet: " fmt, ## __VA_ARGS__); \
> + } \
> + } while (0)
> +
> +#define BADF(fmt, ...) \
> + do { \
> + fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__); \
> + if (DEBUG_STELLARIS_ENET) { \
> + exit(1); \
> + } \
> + } while (0)
>
> #define SE_INT_RX 0x01
> #define SE_INT_TXER 0x02
> --
> 2.8.0.rc3
Thomas