Am 28.04.2014 um 10:26 hat Marc Marí geschrieben: > From: Marc Marí <5.markm...@gmail.com> > > Modify debug macros as explained in > https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03642.html > > Signed-off-by: Marc Marí <5.markm...@gmail.com> > --- > hw/dma/i82374.c | 17 ++++++++++------- > hw/dma/i8257.c | 24 +++++++++++++++++------- > hw/dma/rc4030.c | 13 +++++++++---- > 3 files changed, 36 insertions(+), 18 deletions(-) > > diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c > index dc7a767..fff4e6f 100644 > --- a/hw/dma/i82374.c > +++ b/hw/dma/i82374.c > @@ -24,15 +24,18 @@ > > #include "hw/isa/isa.h" > > -//#define DEBUG_I82374 > +//#define DEBUG_I82374 1 > > -#ifdef DEBUG_I82374 > -#define DPRINTF(fmt, ...) \ > -do { fprintf(stderr, "i82374: " fmt , ## __VA_ARGS__); } while (0) > -#else > -#define DPRINTF(fmt, ...) \ > -do {} while (0) > +#ifndef DEBUG_I82374 > +#define DEBUG_I82374 0 > #endif > + > +#define DPRINTF(fmt, ...) \ > + do { \ > + if(DEBUG_I82374) { \
Coding style: There should be a space in "if (" This seems to be pretty consistent across the whole series. In patch 2, I also saw a "while(0)" without space. I won't comment on each instance of this, just check your whole series for these things when you prepare a v2. > + fprintf(stderr, "I82374: " fmt, ## __VA_ARGS__); \ > + } \ > + } while (0) > #define BADF(fmt, ...) \ > do { fprintf(stderr, "i82374 ERROR: " fmt , ## __VA_ARGS__); } while (0) > > diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c > index 4490372..ac38d7b 100644 > --- a/hw/dma/i8257.c > +++ b/hw/dma/i8257.c > @@ -25,17 +25,27 @@ > #include "hw/isa/isa.h" > #include "qemu/main-loop.h" > > -/* #define DEBUG_DMA */ > +/* #define DEBUG_DMA 1*/ > > #define dolog(...) fprintf (stderr, "dma: " __VA_ARGS__) > -#ifdef DEBUG_DMA > -#define linfo(...) fprintf (stderr, "dma: " __VA_ARGS__) > -#define ldebug(...) fprintf (stderr, "dma: " __VA_ARGS__) > -#else > -#define linfo(...) > -#define ldebug(...) > +#ifndef DEBUG_DMA > +#define DEBUG_DMA 0 > #endif > > +#define linfo(...) \ > + do { \ > + if(DEBUG_DMA) { \ > + fprintf(stderr, "dma: " __VA_ARGS__); \ > + } \ > + } while (0) > + Trailing whitespace. > +#define ldebug(...) \ > + do { \ > + if(DEBUG_DMA) { \ > + fprintf(stderr, "dma: " __VA_ARGS__); \ > + } \ > + } while (0) > + > struct dma_regs { > int now[2]; > uint16_t base[2]; > diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c > index af26632..217b3f3 100644 > --- a/hw/dma/rc4030.c > +++ b/hw/dma/rc4030.c > @@ -29,18 +29,23 @@ > /********************************************************/ > /* debug rc4030 */ > > -//#define DEBUG_RC4030 > +//#define DEBUG_RC4030 1 > //#define DEBUG_RC4030_DMA Should DEBUG_RC4030_DMA be converted as well? It would probably be even more useful than the other #defines because it enables a whole block of code and not just a single printf. Could be a separate patch (series), though. > #ifdef DEBUG_RC4030 > -#define DPRINTF(fmt, ...) \ > -do { printf("rc4030: " fmt , ## __VA_ARGS__); } while (0) > static const char* irq_names[] = { "parallel", "floppy", "sound", "video", > "network", "scsi", "keyboard", "mouse", "serial0", "serial1" }; > #else > -#define DPRINTF(fmt, ...) > +#define DEBUG_RC4030 0 > #endif > > +#define DPRINTF(fmt, ...) \ > + do { \ > + if(DEBUG_RC4030) { \ > + printf("rc4030: " fmt, ## __VA_ARGS__); \ > + } \ > + } while (0) > + > #define RC4030_ERROR(fmt, ...) \ > do { fprintf(stderr, "rc4030 ERROR: %s: " fmt, __func__ , ## __VA_ARGS__); } > while (0) Apart from the style issues and potential extension mentioned above, the logic looks fine. Reviewed-by: Kevin Wolf <kw...@redhat.com>