| Message ID | 20170406111646.12624-4-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 06.04.2017 13:16, Cornelia Huck wrote: > From: Danil Antonov <g.danil.anto@gmail.com> > > 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> > Message-Id: <CA+KKJYBi31Bs7DtVdzZdwG2t+u5+FGiAhQpd3pqJzUX1O8Cprg@mail.gmail.com> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > hw/s390x/s390-pci-bus.c | 16 ++++++++++------ > hw/s390x/s390-pci-inst.c | 16 ++++++++++------ > 2 files changed, 20 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 69b0291e8a..0f62363434 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -24,14 +24,18 @@ > #include "qemu/error-report.h" > > /* #define DEBUG_S390PCI_BUS */ This comment is now somewhat misleading. If you uncomment this "#define DEBUG_S390PCI_BUS" (without adding a "1" at the end), you end up with some empty "if ()" statements now, i.e. compilation failures. So I'd like to suggest to simply remove the above line. > -#ifdef DEBUG_S390PCI_BUS > -#define DPRINTF(fmt, ...) \ > - do { fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); } while (0) > -#else > -#define DPRINTF(fmt, ...) \ > - do { } while (0) > + > +#ifndef DEBUG_S390PCI_BUS > +#define DEBUG_S390PCI_BUS 0 > #endif > > +#define DPRINTF(fmt, ...) \ > + do { \ > + if (DEBUG_S390PCI_BUS) { \ > + fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \ > + } \ > + } while (0) > + > S390pciState *s390_get_phb(void) > { > static S390pciState *phb; > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index d2a8c0a083..763eebd67f 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -21,14 +21,18 @@ > #include "sysemu/hw_accel.h" > > /* #define DEBUG_S390PCI_INST */ dito > -#ifdef DEBUG_S390PCI_INST > -#define DPRINTF(fmt, ...) \ > - do { fprintf(stderr, "s390pci-inst: " fmt, ## __VA_ARGS__); } while (0) > -#else > -#define DPRINTF(fmt, ...) \ > - do { } while (0) > + > +#ifndef DEBUG_S390PCI_INST > +#define DEBUG_S390PCI_INST 0 > #endif > > +#define DPRINTF(fmt, ...) \ > + do { \ > + if (DEBUG_S390PCI_INST) { \ > + fprintf(stderr, "s390pci-inst: " fmt, ## __VA_ARGS__); \ > + } \ > + } while (0) > + > static void s390_set_status_code(CPUS390XState *env, > uint8_t r, uint64_t status_code) > { > Thomas
On Thu, 6 Apr 2017 14:15:45 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 06.04.2017 13:16, Cornelia Huck wrote: > > From: Danil Antonov <g.danil.anto@gmail.com> > > > > 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> > > Message-Id: <CA+KKJYBi31Bs7DtVdzZdwG2t+u5+FGiAhQpd3pqJzUX1O8Cprg@mail.gmail.com> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > hw/s390x/s390-pci-bus.c | 16 ++++++++++------ > > hw/s390x/s390-pci-inst.c | 16 ++++++++++------ > > 2 files changed, 20 insertions(+), 12 deletions(-) > > > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > > index 69b0291e8a..0f62363434 100644 > > --- a/hw/s390x/s390-pci-bus.c > > +++ b/hw/s390x/s390-pci-bus.c > > @@ -24,14 +24,18 @@ > > #include "qemu/error-report.h" > > > > /* #define DEBUG_S390PCI_BUS */ > > This comment is now somewhat misleading. If you uncomment this "#define > DEBUG_S390PCI_BUS" (without adding a "1" at the end), you end up with > some empty "if ()" statements now, i.e. compilation failures. So I'd > like to suggest to simply remove the above line. Yeah, makes sense, and also matches what is done in the s390x/kvm patch. I'll merge that in. > > > -#ifdef DEBUG_S390PCI_BUS > > -#define DPRINTF(fmt, ...) \ > > - do { fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); } while (0) > > -#else > > -#define DPRINTF(fmt, ...) \ > > - do { } while (0) > > + > > +#ifndef DEBUG_S390PCI_BUS > > +#define DEBUG_S390PCI_BUS 0 > > #endif > > > > +#define DPRINTF(fmt, ...) \ > > + do { \ > > + if (DEBUG_S390PCI_BUS) { \ > > + fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \ > > + } \ > > + } while (0) > > +
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 69b0291e8a..0f62363434 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -24,14 +24,18 @@ #include "qemu/error-report.h" /* #define DEBUG_S390PCI_BUS */ -#ifdef DEBUG_S390PCI_BUS -#define DPRINTF(fmt, ...) \ - do { fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); } while (0) -#else -#define DPRINTF(fmt, ...) \ - do { } while (0) + +#ifndef DEBUG_S390PCI_BUS +#define DEBUG_S390PCI_BUS 0 #endif +#define DPRINTF(fmt, ...) \ + do { \ + if (DEBUG_S390PCI_BUS) { \ + fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); \ + } \ + } while (0) + S390pciState *s390_get_phb(void) { static S390pciState *phb; diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index d2a8c0a083..763eebd67f 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -21,14 +21,18 @@ #include "sysemu/hw_accel.h" /* #define DEBUG_S390PCI_INST */ -#ifdef DEBUG_S390PCI_INST -#define DPRINTF(fmt, ...) \ - do { fprintf(stderr, "s390pci-inst: " fmt, ## __VA_ARGS__); } while (0) -#else -#define DPRINTF(fmt, ...) \ - do { } while (0) + +#ifndef DEBUG_S390PCI_INST +#define DEBUG_S390PCI_INST 0 #endif +#define DPRINTF(fmt, ...) \ + do { \ + if (DEBUG_S390PCI_INST) { \ + fprintf(stderr, "s390pci-inst: " fmt, ## __VA_ARGS__); \ + } \ + } while (0) + static void s390_set_status_code(CPUS390XState *env, uint8_t r, uint64_t status_code) {