Patchwork [V12,24/27] pm_smbus: remove #ifdef DEBUG.

login
register
mail settings
Submitter Isaku Yamahata
Date Jan. 6, 2010, 2:39 a.m.
Message ID <1262745591-28697-25-git-send-email-yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/42248/
State New
Headers show

Comments

Isaku Yamahata - Jan. 6, 2010, 2:39 a.m.
remove #ifdef DEBUG by using macro.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pm_smbus.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
Stefan Weil - Jan. 6, 2010, 11:42 a.m.
Isaku Yamahata schrieb:
> remove #ifdef DEBUG by using macro.
>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/pm_smbus.c | 21 ++++++++++++---------
> 1 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
> index 6ef6b9e..9929d72 100644
> --- a/hw/pm_smbus.c
> +++ b/hw/pm_smbus.c
> @@ -37,6 +37,15 @@
> #define SMBHSTDAT1 0x06
> #define SMBBLKDAT 0x07
>
> +//#define DEBUG
> +
> +#ifdef DEBUG
> +# define SMBUS_DPRINTF(format, ...) printf(format, ## __VA_ARGS__)

Debug output should go to stderr. So this would be even better:

+# define SMBUS_DPRINTF(format, ...) fprintf(stderr, format, ## __VA_ARGS__)

Regards,

Stefan Weil
Isaku Yamahata - Jan. 6, 2010, 11:51 p.m.
On Wed, Jan 06, 2010 at 12:42:28PM +0100, Stefan Weil wrote:
> Isaku Yamahata schrieb:
> > remove #ifdef DEBUG by using macro.
> >
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> > hw/pm_smbus.c | 21 ++++++++++++---------
> > 1 files changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
> > index 6ef6b9e..9929d72 100644
> > --- a/hw/pm_smbus.c
> > +++ b/hw/pm_smbus.c
> > @@ -37,6 +37,15 @@
> > #define SMBHSTDAT1 0x06
> > #define SMBBLKDAT 0x07
> >
> > +//#define DEBUG
> > +
> > +#ifdef DEBUG
> > +# define SMBUS_DPRINTF(format, ...) printf(format, ## __VA_ARGS__)
> 
> Debug output should go to stderr. So this would be even better:
> 
> +# define SMBUS_DPRINTF(format, ...) fprintf(stderr, format, ## __VA_ARGS__)
> 

Yes, in general.
However the original code sends debug output to stdout and
Most of debug output goes to stdout than stderr. You can easily
see it by greping with DEBUG.

So at this time, I'd like to send debug output to stdout.
(And hopefully later create a framework and make debug output go
to stderr consistently over sources.)
Stefan Weil - Jan. 17, 2010, 4:09 p.m.
Isaku Yamahata schrieb:
> On Wed, Jan 06, 2010 at 12:42:28PM +0100, Stefan Weil wrote:
>> Isaku Yamahata schrieb:
>>> remove #ifdef DEBUG by using macro.
>>>
>>> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>> hw/pm_smbus.c | 21 ++++++++++++---------
>>> 1 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
>>> index 6ef6b9e..9929d72 100644
>>> --- a/hw/pm_smbus.c
>>> +++ b/hw/pm_smbus.c
>>> @@ -37,6 +37,15 @@
>>> #define SMBHSTDAT1 0x06
>>> #define SMBBLKDAT 0x07
>>>
>>> +//#define DEBUG
>>> +
>>> +#ifdef DEBUG
>>> +# define SMBUS_DPRINTF(format, ...) printf(format, ## __VA_ARGS__)
>> Debug output should go to stderr. So this would be even better:
>>
>> +# define SMBUS_DPRINTF(format, ...) fprintf(stderr, format, ##
>> __VA_ARGS__)
>>
>
> Yes, in general.
> However the original code sends debug output to stdout and
> Most of debug output goes to stdout than stderr. You can easily
> see it by greping with DEBUG.
>
> So at this time, I'd like to send debug output to stdout.
> (And hopefully later create a framework and make debug output go
> to stderr consistently over sources.)

You are correct: there is much code which writes debug output to stdout.

When qemu is running in system mode with curses interface (-curses) or
serial console only (-nographic), debug output and system output will
be mixed on stdout. In many cases, this makes the output unreadable.

Therefore I prefer a separate output channel for debug messages.
The usual (and easiest) solution is directing them to stderr.
This allows mixing normal and debug output (sometimes a useful
feature) or separating them by using standard I/O redirection.

A more advanced solution could provide special logging functions.
Those could write debug messages to qemu.log (or some other file).
Debug messages from drivers could be prefixed using the qdev driver
name, we could support levels (info, warning, error, ....),
more than one output channel - everything typical logging frameworks
like log4c (or others) support.

I suggest these steps:

1. Debug output to stdout is no longer accepted for new / modified code.

2. New or modified debug messages should go to stderr.

3. Patches which switch debug output from stdout to stderr are accepted.

4. We discuss whether debug output to stderr is sufficient
   or which logging framework should be used.

Please note that I talk here about debug log messages
which are activated by conditional compilation and
which are disabled normally (not those going to qemu.log).

Any comments are welcome.

Kind regards,
Stefan Weil
Paul Brook - Feb. 24, 2010, 2:28 a.m.
> I suggest these steps:
> 
> 1. Debug output to stdout is no longer accepted for new / modified code.
> 
> 2. New or modified debug messages should go to stderr.

I don't see this as a real improvement.  Arguably these aren't errors, so 
stdout is where they should be going.

If we're going to do anything sensible with debug output then we should figure 
out how to do it properly and consistently.  Also remember that this isn't 
supposed to be user friendly, it's for internal developer use only.

Paul
Stefan Weil - Feb. 24, 2010, 6:29 p.m.
Paul Brook schrieb:
>> I suggest these steps:
>>
>> 1. Debug output to stdout is no longer accepted for new / modified code.
>>
>> 2. New or modified debug messages should go to stderr.
>
> I don't see this as a real improvement. Arguably these aren't errors, so
> stdout is where they should be going.
>
> If we're going to do anything sensible with debug output then we
> should figure
> out how to do it properly and consistently. Also remember that this isn't
> supposed to be user friendly, it's for internal developer use only.
>
> Paul

stdout is already used by curses or serial console output
when QEMU is called with appropriate command line switches.

If debugging output also goes to stdout, program output and
debugging output are mixed which can make both completely
unreadable.

Just consider debug output from the serial driver (or any other
device which works parallel to the normal program output).
I was already confronted with this problem.

My proposal is easy to implement and sufficient for internal
developer use. It is not new: parts of QEMU already use
stderr for debugging output. Of course better solutions exist,
but they also need more efforts.

Stefan

Patch

diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
index 6ef6b9e..9929d72 100644
--- a/hw/pm_smbus.c
+++ b/hw/pm_smbus.c
@@ -37,6 +37,15 @@ 
 #define SMBHSTDAT1      0x06
 #define SMBBLKDAT       0x07
 
+//#define DEBUG
+
+#ifdef DEBUG
+# define SMBUS_DPRINTF(format, ...)     printf(format, ## __VA_ARGS__)
+#else
+# define SMBUS_DPRINTF(format, ...)     do { } while (0)
+#endif
+
+
 static void smb_transaction(PMSMBus *s)
 {
     uint8_t prot = (s->smb_ctl >> 2) & 0x07;
@@ -45,9 +54,7 @@  static void smb_transaction(PMSMBus *s)
     uint8_t addr = s->smb_addr >> 1;
     i2c_bus *bus = s->smbus;
 
-#ifdef DEBUG
-    printf("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
-#endif
+    SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
     switch(prot) {
     case 0x0:
         smbus_quick_command(bus, addr, read);
@@ -96,9 +103,7 @@  void smb_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
 {
     PMSMBus *s = opaque;
     addr &= 0x3f;
-#ifdef DEBUG
-    printf("SMB writeb port=0x%04x val=0x%02x\n", addr, val);
-#endif
+    SMBUS_DPRINTF("SMB writeb port=0x%04x val=0x%02x\n", addr, val);
     switch(addr) {
     case SMBHSTSTS:
         s->smb_stat = 0;
@@ -166,9 +171,7 @@  uint32_t smb_ioport_readb(void *opaque, uint32_t addr)
         val = 0;
         break;
     }
-#ifdef DEBUG
-    printf("SMB readb port=0x%04x val=0x%02x\n", addr, val);
-#endif
+    SMBUS_DPRINTF("SMB readb port=0x%04x val=0x%02x\n", addr, val);
     return val;
 }