| Message ID | CA+KKJYCUmxtb7o7Fg-DLFaEqhjZk5MhfBjSMt6L146PYmnGWxw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 <g.danil.anto@gmail.com <mailto:g.danil.anto@gmail.com>> > 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 <g.danil.anto@gmail.com > <mailto:g.danil.anto@gmail.com>> ... 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
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index efa33ad..1236990 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -25,13 +25,13 @@ #include "qemu/timer.h" #include <zlib.h> -//#define DEBUG_SONIC +#ifndef DEBUG_SONIC +#define DEBUG_SONIC 0 +#endif #define SONIC_PROM_SIZE 0x1000 #ifdef DEBUG_SONIC -#define DPRINTF(fmt, ...) \ -do { printf("sonic: " fmt , ## __VA_ARGS__); } while (0) static const char* reg_names[] = { "CR", "DCR", "RCR", "TCR", "IMR", "ISR", "UTDA", "CTDA", "TPS", "TFC", "TSA0", "TSA1", "TFS", "URDA", "CRDA", "CRBA0", @@ -41,10 +41,15 @@ static const char* reg_names[] = { "SR", "WT0", "WT1", "RSC", "CRCT", "FAET", "MPT", "MDT", "0x30", "0x31", "0x32", "0x33", "0x34", "0x35", "0x36", "0x37", "0x38", "0x39", "0x3a", "0x3b", "0x3c", "0x3d", "0x3e", "DCR2" }; -#else -#define DPRINTF(fmt, ...) do {} while (0) #endif +#define DPRINTF(fmt, ...) \ + do { \ + if (DEBUG_SONIC) { \ + fprintf(stderr, "sonic: " fmt, ## __VA_ARGS__); \ + } \ + } while (0) + #define SONIC_ERROR(fmt, ...) \ do { printf("sonic ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } while (0) 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 +#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 +#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); } 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); 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 -#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); 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 -#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)