| Message ID | 1400367823-32610-4-git-send-email-marc.mari.barcelo@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, May 18, 2014 at 9:03 AM, Marc Marí <marc.mari.barcelo@gmail.com> wrote: > Modify debug macros to have the same format through the codebase and use regular > ifs instead of ifdef. > > Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com> > --- > hw/s390x/s390-virtio-bus.c | 9 +++++---- > hw/s390x/s390-virtio.c | 9 +++++---- > target-s390x/helper.c | 23 +++++++++++++++-------- > target-s390x/kvm.c | 9 +++++---- > 4 files changed, 30 insertions(+), 20 deletions(-) > > diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > index 9c71afa..2a1799e 100644 > --- a/hw/s390x/s390-virtio-bus.c > +++ b/hw/s390x/s390-virtio-bus.c > @@ -38,13 +38,14 @@ > /* #define DEBUG_S390 */ > > #ifdef DEBUG_S390 > -#define DPRINTF(fmt, ...) \ > - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) > +#define DEBUG_S390_ENABLED 1 > #else > -#define DPRINTF(fmt, ...) \ > - do { } while (0) > +#define DEBUG_S390_ENABLED 0 > #endif > > +#define DPRINTF(fmt, ...) \ > + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio bus", fmt, ## __VA_ARGS__) > + > #define VIRTIO_EXT_CODE 0x2603 > > static void virtio_s390_bus_new(VirtioBusState *bus, size_t bus_size, > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c > index aef2003..b69afb4 100644 > --- a/hw/s390x/s390-virtio.c > +++ b/hw/s390x/s390-virtio.c > @@ -42,13 +42,14 @@ > //#define DEBUG_S390 > > #ifdef DEBUG_S390 > -#define DPRINTF(fmt, ...) \ > - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) > +#define DEBUG_S390_ENABLED 1 > #else > -#define DPRINTF(fmt, ...) \ > - do { } while (0) > +#define DEBUG_S390_ENABLED 0 > #endif > > +#define DPRINTF(fmt, ...) \ > + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio", fmt, ## __VA_ARGS__) > + > #define MAX_BLK_DEVS 10 > #define ZIPL_FILENAME "s390-zipl.rom" > > diff --git a/target-s390x/helper.c b/target-s390x/helper.c > index 7c76fc1..c2aa568 100644 > --- a/target-s390x/helper.c > +++ b/target-s390x/helper.c > @@ -30,19 +30,26 @@ > //#define DEBUG_S390_STDOUT > > #ifdef DEBUG_S390 > -#ifdef DEBUG_S390_STDOUT > -#define DPRINTF(fmt, ...) \ > - do { fprintf(stderr, fmt, ## __VA_ARGS__); \ > - qemu_log(fmt, ##__VA_ARGS__); } while (0) > +#define DEBUG_S390_ENABLED 1 > #else > -#define DPRINTF(fmt, ...) \ > - do { qemu_log(fmt, ## __VA_ARGS__); } while (0) > +#define DEBUG_S390_ENABLED 0 > #endif > + > +#ifdef DEBUG_S390_STDOUT This STDOUT vs _log() choice is a bit irregular, and I think you might be better off abandoning it completely. Richard, Alex, do we really need to optionally route printfery to log or stderr? (considering _log is NOW stderr by default now, and -D option gives you some flexibility there). Can we have just log and drop STDOUT mode? Regards, Peter > +#define DEBUG_S390_STDOUT_ENABLED 1 > #else > -#define DPRINTF(fmt, ...) \ > - do { } while (0) > +#define DEBUG_S390_STDOUT_ENABLED 0 > #endif > > +#define DPRINTF(fmt, ...) \ > + do { \ > + if(DEBUG_S390_ENABLED) { \ > + qemu_log(fmt, ##__VA_ARGS__); \ > + QEMU_DPRINTF(DEBUG_S390_STDOUT_ENABLED, "s390x helper", \ > + fmt, ## __VA_ARGS__); \ > + } \ > + } while (0) > + > #ifdef DEBUG_S390_PTE > #define PTE_DPRINTF DPRINTF > #else > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c > index 56179af..63c46c4 100644 > --- a/target-s390x/kvm.c > +++ b/target-s390x/kvm.c > @@ -41,13 +41,14 @@ > /* #define DEBUG_KVM */ > > #ifdef DEBUG_KVM > -#define DPRINTF(fmt, ...) \ > - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) > +#define DEBUG_KVM_ENABLED 1 > #else > -#define DPRINTF(fmt, ...) \ > - do { } while (0) > +#define DEBUG_KVM_ENABLED 0 > #endif > > +#define DPRINTF(fmt, ...) \ > + QEMU_DPRINTF(DEBUG_KVM_ENABLED, "s390 kvm", fmt, ## __VA_ARGS__) > + > #define IPA0_DIAG 0x8300 > #define IPA0_SIGP 0xae00 > #define IPA0_B2 0xb200 > -- > 1.7.10.4 > >
On 18.05.14 11:37, Peter Crosthwaite wrote: > On Sun, May 18, 2014 at 9:03 AM, Marc Marí <marc.mari.barcelo@gmail.com> wrote: >> Modify debug macros to have the same format through the codebase and use regular >> ifs instead of ifdef. >> >> Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com> >> --- >> hw/s390x/s390-virtio-bus.c | 9 +++++---- >> hw/s390x/s390-virtio.c | 9 +++++---- >> target-s390x/helper.c | 23 +++++++++++++++-------- >> target-s390x/kvm.c | 9 +++++---- >> 4 files changed, 30 insertions(+), 20 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c >> index 9c71afa..2a1799e 100644 >> --- a/hw/s390x/s390-virtio-bus.c >> +++ b/hw/s390x/s390-virtio-bus.c >> @@ -38,13 +38,14 @@ >> /* #define DEBUG_S390 */ >> >> #ifdef DEBUG_S390 >> -#define DPRINTF(fmt, ...) \ >> - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) >> +#define DEBUG_S390_ENABLED 1 >> #else >> -#define DPRINTF(fmt, ...) \ >> - do { } while (0) >> +#define DEBUG_S390_ENABLED 0 >> #endif >> >> +#define DPRINTF(fmt, ...) \ >> + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio bus", fmt, ## __VA_ARGS__) >> + >> #define VIRTIO_EXT_CODE 0x2603 >> >> static void virtio_s390_bus_new(VirtioBusState *bus, size_t bus_size, >> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c >> index aef2003..b69afb4 100644 >> --- a/hw/s390x/s390-virtio.c >> +++ b/hw/s390x/s390-virtio.c >> @@ -42,13 +42,14 @@ >> //#define DEBUG_S390 >> >> #ifdef DEBUG_S390 >> -#define DPRINTF(fmt, ...) \ >> - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) >> +#define DEBUG_S390_ENABLED 1 >> #else >> -#define DPRINTF(fmt, ...) \ >> - do { } while (0) >> +#define DEBUG_S390_ENABLED 0 >> #endif >> >> +#define DPRINTF(fmt, ...) \ >> + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio", fmt, ## __VA_ARGS__) >> + >> #define MAX_BLK_DEVS 10 >> #define ZIPL_FILENAME "s390-zipl.rom" >> >> diff --git a/target-s390x/helper.c b/target-s390x/helper.c >> index 7c76fc1..c2aa568 100644 >> --- a/target-s390x/helper.c >> +++ b/target-s390x/helper.c >> @@ -30,19 +30,26 @@ >> //#define DEBUG_S390_STDOUT >> >> #ifdef DEBUG_S390 >> -#ifdef DEBUG_S390_STDOUT >> -#define DPRINTF(fmt, ...) \ >> - do { fprintf(stderr, fmt, ## __VA_ARGS__); \ >> - qemu_log(fmt, ##__VA_ARGS__); } while (0) >> +#define DEBUG_S390_ENABLED 1 >> #else >> -#define DPRINTF(fmt, ...) \ >> - do { qemu_log(fmt, ## __VA_ARGS__); } while (0) >> +#define DEBUG_S390_ENABLED 0 >> #endif >> + >> +#ifdef DEBUG_S390_STDOUT > This STDOUT vs _log() choice is a bit irregular, and I think you might > be better off abandoning it completely. Richard, Alex, do we really > need to optionally route printfery to log or stderr? (considering _log > is NOW stderr by default now, and -D option gives you some flexibility > there). Can we have just log and drop STDOUT mode? Works for me :). Alex
El Sun, 18 May 2014 19:37:12 +1000 Peter Crosthwaite <peter.crosthwaite@xilinx.com> escribió: > On Sun, May 18, 2014 at 9:03 AM, Marc Marí > <marc.mari.barcelo@gmail.com> wrote: > > Modify debug macros to have the same format through the codebase > > and use regular ifs instead of ifdef. > > > > Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com> > > --- > > hw/s390x/s390-virtio-bus.c | 9 +++++---- > > hw/s390x/s390-virtio.c | 9 +++++---- > > target-s390x/helper.c | 23 +++++++++++++++-------- > > target-s390x/kvm.c | 9 +++++---- > > 4 files changed, 30 insertions(+), 20 deletions(-) > > > > diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > > index 9c71afa..2a1799e 100644 > > --- a/hw/s390x/s390-virtio-bus.c > > +++ b/hw/s390x/s390-virtio-bus.c > > @@ -38,13 +38,14 @@ > > /* #define DEBUG_S390 */ > > > > #ifdef DEBUG_S390 > > -#define DPRINTF(fmt, ...) \ > > - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) > > +#define DEBUG_S390_ENABLED 1 > > #else > > -#define DPRINTF(fmt, ...) \ > > - do { } while (0) > > +#define DEBUG_S390_ENABLED 0 > > #endif > > > > +#define DPRINTF(fmt, ...) \ > > + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio bus", fmt, ## > > __VA_ARGS__) + > > #define VIRTIO_EXT_CODE 0x2603 > > > > static void virtio_s390_bus_new(VirtioBusState *bus, size_t > > bus_size, diff --git a/hw/s390x/s390-virtio.c > > b/hw/s390x/s390-virtio.c index aef2003..b69afb4 100644 > > --- a/hw/s390x/s390-virtio.c > > +++ b/hw/s390x/s390-virtio.c > > @@ -42,13 +42,14 @@ > > //#define DEBUG_S390 > > > > #ifdef DEBUG_S390 > > -#define DPRINTF(fmt, ...) \ > > - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) > > +#define DEBUG_S390_ENABLED 1 > > #else > > -#define DPRINTF(fmt, ...) \ > > - do { } while (0) > > +#define DEBUG_S390_ENABLED 0 > > #endif > > > > +#define DPRINTF(fmt, ...) \ > > + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio", fmt, ## > > __VA_ARGS__) + > > #define MAX_BLK_DEVS 10 > > #define ZIPL_FILENAME "s390-zipl.rom" > > > > diff --git a/target-s390x/helper.c b/target-s390x/helper.c > > index 7c76fc1..c2aa568 100644 > > --- a/target-s390x/helper.c > > +++ b/target-s390x/helper.c > > @@ -30,19 +30,26 @@ > > //#define DEBUG_S390_STDOUT > > > > #ifdef DEBUG_S390 > > -#ifdef DEBUG_S390_STDOUT > > -#define DPRINTF(fmt, ...) \ > > - do { fprintf(stderr, fmt, ## __VA_ARGS__); \ > > - qemu_log(fmt, ##__VA_ARGS__); } while (0) > > +#define DEBUG_S390_ENABLED 1 > > #else > > -#define DPRINTF(fmt, ...) \ > > - do { qemu_log(fmt, ## __VA_ARGS__); } while (0) > > +#define DEBUG_S390_ENABLED 0 > > #endif > > + > > +#ifdef DEBUG_S390_STDOUT > > This STDOUT vs _log() choice is a bit irregular, and I think you might > be better off abandoning it completely. Richard, Alex, do we really > need to optionally route printfery to log or stderr? (considering _log > is NOW stderr by default now, and -D option gives you some flexibility > there). Can we have just log and drop STDOUT mode? > > Regards, > Peter QEMU_DPRINTF outputs to stderr, and, as you say qemu_log does it too. Should QEMU_DPRINTF be removed and leave only qemu_log? Marc
On 05/19/2014 09:45 AM, Marc Marí wrote: >> > This STDOUT vs _log() choice is a bit irregular, and I think you might >> > be better off abandoning it completely. Richard, Alex, do we really >> > need to optionally route printfery to log or stderr? (considering _log >> > is NOW stderr by default now, and -D option gives you some flexibility >> > there). Can we have just log and drop STDOUT mode? >> > >> > Regards, >> > Peter > QEMU_DPRINTF outputs to stderr, and, as you say qemu_log does it too. > Should QEMU_DPRINTF be removed and leave only qemu_log? Yes, I think that all debug output like this should go via qemu_log. r~
On Tue, May 20, 2014 at 2:50 AM, Richard Henderson <rth@twiddle.net> wrote: > On 05/19/2014 09:45 AM, Marc Marí wrote: >>> > This STDOUT vs _log() choice is a bit irregular, and I think you might >>> > be better off abandoning it completely. Richard, Alex, do we really >>> > need to optionally route printfery to log or stderr? (considering _log >>> > is NOW stderr by default now, and -D option gives you some flexibility >>> > there). Can we have just log and drop STDOUT mode? >>> > >>> > Regards, >>> > Peter >> QEMU_DPRINTF outputs to stderr, and, as you say qemu_log does it too. >> Should QEMU_DPRINTF be removed and leave only qemu_log? Ahh I see now, you want to use the common factored-out code in P1 which is stderr specific. Perhaps that should use qemu_log. One option would be to create a variant of it in common code that does uses qemu_log instead of stderr and use that variant here. That way you can preserve existing behaviour (and maintainer expectations), as much as possible. > > Yes, I think that all debug output like this should go via qemu_log. > Thanks. Regards, Peter > > r~ >
On 19.05.14 22:33, Peter Crosthwaite wrote: > On Tue, May 20, 2014 at 2:50 AM, Richard Henderson <rth@twiddle.net> wrote: >> On 05/19/2014 09:45 AM, Marc Marí wrote: >>>>> This STDOUT vs _log() choice is a bit irregular, and I think you might >>>>> be better off abandoning it completely. Richard, Alex, do we really >>>>> need to optionally route printfery to log or stderr? (considering _log >>>>> is NOW stderr by default now, and -D option gives you some flexibility >>>>> there). Can we have just log and drop STDOUT mode? >>>>> >>>>> Regards, >>>>> Peter >>> QEMU_DPRINTF outputs to stderr, and, as you say qemu_log does it too. >>> Should QEMU_DPRINTF be removed and leave only qemu_log? > Ahh I see now, you want to use the common factored-out code in P1 > which is stderr specific. Perhaps that should use qemu_log. One option > would be to create a variant of it in common code that does uses > qemu_log instead of stderr and use that variant here. That way you can > preserve existing behaviour (and maintainer expectations), as much as > possible. I think it's safe to assume that all TCG code wants to use qemu_log. Alex
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c index 9c71afa..2a1799e 100644 --- a/hw/s390x/s390-virtio-bus.c +++ b/hw/s390x/s390-virtio-bus.c @@ -38,13 +38,14 @@ /* #define DEBUG_S390 */ #ifdef DEBUG_S390 -#define DPRINTF(fmt, ...) \ - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) +#define DEBUG_S390_ENABLED 1 #else -#define DPRINTF(fmt, ...) \ - do { } while (0) +#define DEBUG_S390_ENABLED 0 #endif +#define DPRINTF(fmt, ...) \ + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio bus", fmt, ## __VA_ARGS__) + #define VIRTIO_EXT_CODE 0x2603 static void virtio_s390_bus_new(VirtioBusState *bus, size_t bus_size, diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c index aef2003..b69afb4 100644 --- a/hw/s390x/s390-virtio.c +++ b/hw/s390x/s390-virtio.c @@ -42,13 +42,14 @@ //#define DEBUG_S390 #ifdef DEBUG_S390 -#define DPRINTF(fmt, ...) \ - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) +#define DEBUG_S390_ENABLED 1 #else -#define DPRINTF(fmt, ...) \ - do { } while (0) +#define DEBUG_S390_ENABLED 0 #endif +#define DPRINTF(fmt, ...) \ + QEMU_DPRINTF(DEBUG_S390_ENABLED, "s390 virtio", fmt, ## __VA_ARGS__) + #define MAX_BLK_DEVS 10 #define ZIPL_FILENAME "s390-zipl.rom" diff --git a/target-s390x/helper.c b/target-s390x/helper.c index 7c76fc1..c2aa568 100644 --- a/target-s390x/helper.c +++ b/target-s390x/helper.c @@ -30,19 +30,26 @@ //#define DEBUG_S390_STDOUT #ifdef DEBUG_S390 -#ifdef DEBUG_S390_STDOUT -#define DPRINTF(fmt, ...) \ - do { fprintf(stderr, fmt, ## __VA_ARGS__); \ - qemu_log(fmt, ##__VA_ARGS__); } while (0) +#define DEBUG_S390_ENABLED 1 #else -#define DPRINTF(fmt, ...) \ - do { qemu_log(fmt, ## __VA_ARGS__); } while (0) +#define DEBUG_S390_ENABLED 0 #endif + +#ifdef DEBUG_S390_STDOUT +#define DEBUG_S390_STDOUT_ENABLED 1 #else -#define DPRINTF(fmt, ...) \ - do { } while (0) +#define DEBUG_S390_STDOUT_ENABLED 0 #endif +#define DPRINTF(fmt, ...) \ + do { \ + if(DEBUG_S390_ENABLED) { \ + qemu_log(fmt, ##__VA_ARGS__); \ + QEMU_DPRINTF(DEBUG_S390_STDOUT_ENABLED, "s390x helper", \ + fmt, ## __VA_ARGS__); \ + } \ + } while (0) + #ifdef DEBUG_S390_PTE #define PTE_DPRINTF DPRINTF #else diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c index 56179af..63c46c4 100644 --- a/target-s390x/kvm.c +++ b/target-s390x/kvm.c @@ -41,13 +41,14 @@ /* #define DEBUG_KVM */ #ifdef DEBUG_KVM -#define DPRINTF(fmt, ...) \ - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) +#define DEBUG_KVM_ENABLED 1 #else -#define DPRINTF(fmt, ...) \ - do { } while (0) +#define DEBUG_KVM_ENABLED 0 #endif +#define DPRINTF(fmt, ...) \ + QEMU_DPRINTF(DEBUG_KVM_ENABLED, "s390 kvm", fmt, ## __VA_ARGS__) + #define IPA0_DIAG 0x8300 #define IPA0_SIGP 0xae00 #define IPA0_B2 0xb200
Modify debug macros to have the same format through the codebase and use regular ifs instead of ifdef. Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com> --- hw/s390x/s390-virtio-bus.c | 9 +++++---- hw/s390x/s390-virtio.c | 9 +++++---- target-s390x/helper.c | 23 +++++++++++++++-------- target-s390x/kvm.c | 9 +++++---- 4 files changed, 30 insertions(+), 20 deletions(-)