[Qemu-devel] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[Qemu-devel] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[liguang]

when enable DEBUG_DEBUGCON, there are some message
printing bugs, so fix them.

this patch-set based on previous 3 patches,
http://comments.gmane.org/gmane.comp.emulators.qemu/212550
http://comments.gmane.org/gmane.comp.emulators.qemu/212551
http://comments.gmane.org/gmane.comp.emulators.qemu/212552

cover-letter and patch [4/4] are new, no changes
for other 3.

Li Guang (4)
	 debugcon: fix always print "addr=0x0, val=0x0" bug when use DEBUG_DEBUGCON
	 debugcon: make debug message more readable
	 debugcon: fix compiler warning when open DEBUG_DEBUGCON
	 debugcon: use fprintf(stderr...) instead of printf

hw/char/debugcon.c | 13 +-
1 files changed, 7 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/4] debugcon: fix always print "addr=0x0, val=0x0" bug when use DEBUG_DEBUGCON

[liguang]

when use DEBUG_DEBUGCON, screen spits:
debugcon: write addr=0x0000 val=0x00
Rdebugcon: write addr=0x0000 val=0x00
udebugcon: write addr=0x0000 val=0x00
ndebugcon: write addr=0x0000 val=0x00
ndebugcon: write addr=0x0000 val=0x00
idebugcon: write addr=0x0000 val=0x00
ndebugcon: write addr=0x0000 val=0x00
gdebugcon: write addr=0x0000 val=0x00
 debugcon: write addr=0x0000 val=0x00
odebugcon: write addr=0x0000 val=0x00
pdebugcon: write addr=0x0000 val=0x00
tdebugcon: write addr=0x0000 val=0x00
idebugcon: write addr=0x0000 val=0x00
odebugcon: write addr=0x0000 val=0x00
ndebugcon: write addr=0x0000 val=0x00
 debugcon: write addr=0x0000 val=0x00
rdebugcon: write addr=0x0000 val=0x00
odebugcon: write addr=0x0000 val=0x00
mdebugcon: write addr=0x0000 val=0x00
 debugcon: write addr=0x0000 val=0x00
adebugcon: write addr=0x0000 val=0x00
tdebugcon: write addr=0x0000 val=0x00
 debugcon: write addr=0x0000 val=0x00

Oh, that's wrong, val is not always be 0.
this bug caused by lack of length modifier
for specifier 'x'.

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/char/debugcon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
index 02c9577..7e41c90 100644
--- a/hw/char/debugcon.c
+++ b/hw/char/debugcon.c
@@ -55,7 +55,7 @@ static void debugcon_ioport_write(void *opaque, hwaddr addr, uint64_t val,
     unsigned char ch = val;
 
 #ifdef DEBUG_DEBUGCON
-    printf("debugcon: write addr=0x%04x val=0x%02x\n", addr, val);
+    printf("debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02x\n", addr, val);
 #endif
 
     qemu_chr_fe_write(s->chr, &ch, 1);
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/4] debugcon: make debug message more readable

[liguang]

before change:
Bdebugcon: write addr=0x0000 val=0x6f
odebugcon: write addr=0x0000 val=0x6f
odebugcon: write addr=0x0000 val=0x74
tdebugcon: write addr=0x0000 val=0x69
idebugcon: write addr=0x0000 val=0x6e
ndebugcon: write addr=0x0000 val=0x67
gdebugcon: write addr=0x0000 val=0x20
 debugcon: write addr=0x0000 val=0x66

after change:
B [debugcon: write addr=0x0000 val=0x6f]
o [debugcon: write addr=0x0000 val=0x6f]
o [debugcon: write addr=0x0000 val=0x74]
t [debugcon: write addr=0x0000 val=0x69]
i [debugcon: write addr=0x0000 val=0x6e]
n [debugcon: write addr=0x0000 val=0x67]
g [debugcon: write addr=0x0000 val=0x20]
  [debugcon: write addr=0x0000 val=0x66]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/char/debugcon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
index 7e41c90..52fa0ab 100644
--- a/hw/char/debugcon.c
+++ b/hw/char/debugcon.c
@@ -55,7 +55,7 @@ static void debugcon_ioport_write(void *opaque, hwaddr addr, uint64_t val,
     unsigned char ch = val;
 
 #ifdef DEBUG_DEBUGCON
-    printf("debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02x\n", addr, val);
+    printf(" [debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02x]\n", addr, val);
 #endif
 
     qemu_chr_fe_write(s->chr, &ch, 1);
-- 
1.7.2.5

[Qemu-devel] [PATCH 3/4] debugcon: fix compiler warning when open DEBUG_DEBUGCON

[liguang]

compiler warnings:
  CC    hw/char/debugcon.o
hw/char/debugcon.c: In function ‘debugcon_ioport_write’:
hw/char/debugcon.c:58: warning: format ‘%02x’ expects type ‘unsigned int’, but argument 3 has type ‘uint64_t’
hw/char/debugcon.c: In function ‘debugcon_ioport_read’:
hw/char/debugcon.c:70: warning: format ‘%04x’ expects type ‘unsigned int’, but argument 2 has type ‘hwaddr’

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/char/debugcon.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
index 52fa0ab..3b0637d 100644
--- a/hw/char/debugcon.c
+++ b/hw/char/debugcon.c
@@ -55,7 +55,7 @@ static void debugcon_ioport_write(void *opaque, hwaddr addr, uint64_t val,
     unsigned char ch = val;
 
 #ifdef DEBUG_DEBUGCON
-    printf(" [debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02x]\n", addr, val);
+    printf(" [debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02" PRIx64 "]\n", addr, val);
 #endif
 
     qemu_chr_fe_write(s->chr, &ch, 1);
@@ -67,7 +67,7 @@ static uint64_t debugcon_ioport_read(void *opaque, hwaddr addr, unsigned width)
     DebugconState *s = opaque;
 
 #ifdef DEBUG_DEBUGCON
-    printf("debugcon: read addr=0x%04x\n", addr);
+    printf("debugcon: read addr=0x%04" HWADDR_PRIx "\n", addr);
 #endif
 
     return s->readback;
-- 
1.7.2.5

[Qemu-devel] [PATCH 4/4] debugcon: use fprintf(stderr...) instead of printf

[liguang]

suggested by Andreas Färber <afaerber@suse.de>

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/char/debugcon.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
index 3b0637d..be20ede 100644
--- a/hw/char/debugcon.c
+++ b/hw/char/debugcon.c
@@ -55,7 +55,8 @@ static void debugcon_ioport_write(void *opaque, hwaddr addr, uint64_t val,
     unsigned char ch = val;
 
 #ifdef DEBUG_DEBUGCON
-    printf(" [debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02" PRIx64 "]\n", addr, val);
+    fprintf(stderr, " [debugcon: write addr=0x%04" HWADDR_PRIx
+            " val=0x%02" PRIx64 "]\n", addr, val);
 #endif
 
     qemu_chr_fe_write(s->chr, &ch, 1);
@@ -67,7 +68,7 @@ static uint64_t debugcon_ioport_read(void *opaque, hwaddr addr, unsigned width)
     DebugconState *s = opaque;
 
 #ifdef DEBUG_DEBUGCON
-    printf("debugcon: read addr=0x%04" HWADDR_PRIx "\n", addr);
+    fprintf(stderr, "debugcon: read addr=0x%04" HWADDR_PRIx "\n", addr);
 #endif
 
     return s->readback;
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 4/4] debugcon: use fprintf(stderr...) instead of printf

[Andreas Färber]

Am 24.05.2013 04:47, schrieb liguang:
> suggested by Andreas Färber <afaerber@suse.de>
> 
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  hw/char/debugcon.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
> index 3b0637d..be20ede 100644
> --- a/hw/char/debugcon.c
> +++ b/hw/char/debugcon.c
> @@ -55,7 +55,8 @@ static void debugcon_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>      unsigned char ch = val;
>  
>  #ifdef DEBUG_DEBUGCON
> -    printf(" [debugcon: write addr=0x%04" HWADDR_PRIx " val=0x%02" PRIx64 "]\n", addr, val);
> +    fprintf(stderr, " [debugcon: write addr=0x%04" HWADDR_PRIx
> +            " val=0x%02" PRIx64 "]\n", addr, val);
>  #endif
>  
>      qemu_chr_fe_write(s->chr, &ch, 1);
> @@ -67,7 +68,7 @@ static uint64_t debugcon_ioport_read(void *opaque, hwaddr addr, unsigned width)
>      DebugconState *s = opaque;
>  
>  #ifdef DEBUG_DEBUGCON
> -    printf("debugcon: read addr=0x%04" HWADDR_PRIx "\n", addr);
> +    fprintf(stderr, "debugcon: read addr=0x%04" HWADDR_PRIx "\n", addr);
>  #endif
>  
>      return s->readback;
> 

Actually my suggestion was to make this a two-patch series:

1/2 debugcon: Fix debug format specifiers
2/2 debugcon: Separate debug console output from device debug output

The former would do both HWADDR_PRIx and PRIx64 in one go and not do a
lengthy example or "Oh, that's wrong" in the commit message.
The latter would change where/how the debug output is written, without
changing to " [...]".

Thinking more about this, I think it would be best to turn this into a
DPRINTF() macro or so, then the change can be made in one central
location and the inline #ifdefs can be dropped. Bonus points if the
macro were designed in such a way that even if DEBUG_DEBUGCON is not
defined the (f)printfs get tested, to avoid regressions. :)

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[Michael Tokarev]

24.05.2013 06:47, liguang wrote:
> when enable DEBUG_DEBUGCON, there are some message
> printing bugs, so fix them.
> 
> this patch-set based on previous 3 patches,
> http://comments.gmane.org/gmane.comp.emulators.qemu/212550
> http://comments.gmane.org/gmane.comp.emulators.qemu/212551
> http://comments.gmane.org/gmane.comp.emulators.qemu/212552
> 
> cover-letter and patch [4/4] are new, no changes
> for other 3.
> 
> Li Guang (4)
> 	 debugcon: fix always print "addr=0x0, val=0x0" bug when use DEBUG_DEBUGCON
> 	 debugcon: make debug message more readable
> 	 debugcon: fix compiler warning when open DEBUG_DEBUGCON
> 	 debugcon: use fprintf(stderr...) instead of printf

The subjects and commit messages are a bit inaccurate, and
as Andreas says, these may have been merged together, but
the inaccuracy is small (and mostly due to language barriers),
and the result is good anyway, with one patch doing one thing.

So.. thanks, applied to the trivial-patches queue.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[Andreas Färber]

Am 25.05.2013 11:28, schrieb Michael Tokarev:
> 24.05.2013 06:47, liguang wrote:
>> when enable DEBUG_DEBUGCON, there are some message
>> printing bugs, so fix them.
>>
>> this patch-set based on previous 3 patches,
>> http://comments.gmane.org/gmane.comp.emulators.qemu/212550
>> http://comments.gmane.org/gmane.comp.emulators.qemu/212551
>> http://comments.gmane.org/gmane.comp.emulators.qemu/212552
>>
>> cover-letter and patch [4/4] are new, no changes
>> for other 3.
>>
>> Li Guang (4)
>> 	 debugcon: fix always print "addr=0x0, val=0x0" bug when use DEBUG_DEBUGCON
>> 	 debugcon: make debug message more readable
>> 	 debugcon: fix compiler warning when open DEBUG_DEBUGCON
>> 	 debugcon: use fprintf(stderr...) instead of printf
> 
> The subjects and commit messages are a bit inaccurate, and
> as Andreas says, these may have been merged together, but
> the inaccuracy is small (and mostly due to language barriers),
> and the result is good anyway, with one patch doing one thing.
> 
> So.. thanks, applied to the trivial-patches queue.

NACK. If you want to apply 1-3, okay. But please unqueue 4/4, it makes
no sense as-is (just look at the stderr output to see what I mean) and
it pretends that I suggested that!

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[Michael Tokarev]

25.05.2013 14:35, Andreas Färber wrote:
> Am 25.05.2013 11:28, schrieb Michael Tokarev:
[]
>>> 	 debugcon: use fprintf(stderr...) instead of printf
>>
>> The subjects and commit messages are a bit inaccurate, and
>> as Andreas says, these may have been merged together, but
>> the inaccuracy is small (and mostly due to language barriers),
>> and the result is good anyway, with one patch doing one thing.
>>
>> So.. thanks, applied to the trivial-patches queue.
> 
> NACK. If you want to apply 1-3, okay. But please unqueue 4/4, it makes
> no sense as-is (just look at the stderr output to see what I mean) and
> it pretends that I suggested that!

Actually I did just that, rebuild with DEBUG_DEBUGCON and looked
at the output, -- because I didn't know how it works.  And you're
right, it's not a good change ;)  But your email come before I was
able to reply.  Unqueued.

Thank you for noticing this.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[li guang]

在 2013-05-25六的 14:42 +0400，Michael Tokarev写道：
> 25.05.2013 14:35, Andreas Färber wrote:
> > Am 25.05.2013 11:28, schrieb Michael Tokarev:
> []
> >>> 	 debugcon: use fprintf(stderr...) instead of printf
> >>
> >> The subjects and commit messages are a bit inaccurate, and
> >> as Andreas says, these may have been merged together, but
> >> the inaccuracy is small (and mostly due to language barriers),
> >> and the result is good anyway, with one patch doing one thing.
> >>
> >> So.. thanks, applied to the trivial-patches queue.
> > 
> > NACK. If you want to apply 1-3, okay. But please unqueue 4/4, it makes
> > no sense as-is (just look at the stderr output to see what I mean) and
> > it pretends that I suggested that!
> 
> Actually I did just that, rebuild with DEBUG_DEBUGCON and looked
> at the output, -- because I didn't know how it works.  And you're
> right, it's not a good change ;)  But your email come before I was
> able to reply.  Unqueued.

Hi, Michael

do you queued patch 1-3?
if so, I will only send one patch for comments from Andreas,
otherwise, I will refactor all patches.

Thanks!

Re: [Qemu-devel] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[Gerd Hoffmann]

On 05/24/13 04:47, liguang wrote:
> when enable DEBUG_DEBUGCON, there are some message
> printing bugs, so fix them.

I'd suggest to either simply remove the debug printfs or turn them into
tracepoints.

cheers,
  Gerd

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[Michael Tokarev]

27.05.2013 05:40, li guang wrote:
[]
>>> NACK. If you want to apply 1-3, okay. But please unqueue 4/4, it makes
>>> no sense as-is (just look at the stderr output to see what I mean) and
>>> it pretends that I suggested that!
>>
>> Actually I did just that, rebuild with DEBUG_DEBUGCON and looked
>> at the output, -- because I didn't know how it works.  And you're
>> right, it's not a good change ;)  But your email come before I was
>> able to reply.  Unqueued.
> 
> Hi, Michael

Hello.  Please excuse me for the long(ish) delay, I was out of the city
when you sent this email and when you pinged me on IRC.

> do you queued patch 1-3?
> if so, I will only send one patch for comments from Andreas,
> otherwise, I will refactor all patches.

Yes I queued your patches 1-3 but not 4.  It can be seen at
http://git.corpit.ru/?p=qemu.git;a=shortlog;h=refs/heads/trivial-patches-next

(I just rebased it again on top of current qemu/master, but your 3
patches are there).

Should I replace these 3 with the next set?

But as Gerd correctly say, maybe it's better to just get rid of
these stuff completely, or maybe, just maybe, replace it with
tracepoints.

Anyway, I think the 3 queued-up patches are okay.

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[li guang]

在 2013-05-28二的 00:42 +0400，Michael Tokarev写道：
> 27.05.2013 05:40, li guang wrote:
> []
> >>> NACK. If you want to apply 1-3, okay. But please unqueue 4/4, it makes
> >>> no sense as-is (just look at the stderr output to see what I mean) and
> >>> it pretends that I suggested that!
> >>
> >> Actually I did just that, rebuild with DEBUG_DEBUGCON and looked
> >> at the output, -- because I didn't know how it works.  And you're
> >> right, it's not a good change ;)  But your email come before I was
> >> able to reply.  Unqueued.
> > 
> > Hi, Michael
> 
> Hello.  Please excuse me for the long(ish) delay, I was out of the city
> when you sent this email and when you pinged me on IRC.

That's OK.

> 
> > do you queued patch 1-3?
> > if so, I will only send one patch for comments from Andreas,
> > otherwise, I will refactor all patches.
> 
> Yes I queued your patches 1-3 but not 4.  It can be seen at
> http://git.corpit.ru/?p=qemu.git;a=shortlog;h=refs/heads/trivial-patches-next
> 
> (I just rebased it again on top of current qemu/master, but your 3
> patches are there).
> 
> Should I replace these 3 with the next set?

I think you don't have to do that.
I will try to do a patch base on the patches you queued
as Andreas commented.

> 
> But as Gerd correctly say, maybe it's better to just get rid of
> these stuff completely, or maybe, just maybe, replace it with
> tracepoints.
> 
> Anyway, I think the 3 queued-up patches are okay.
> 

I will consider this comments later.

Thanks!

Re: [Qemu-devel] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[li guang]

在 2013-05-27一的 11:13 +0200，Gerd Hoffmann写道：
> On 05/24/13 04:47, liguang wrote:
> > when enable DEBUG_DEBUGCON, there are some message
> > printing bugs, so fix them.
> 
> I'd suggest to either simply remove the debug printfs or turn them into
> tracepoints.
> 

sorry, why we simply remove debug message printing?
it's un-useful at all?

but, for me, it is helpful.

Thanks!

Re: [Qemu-devel] [PATCH 0/4] debugcon: fix some bugs when DEBUG_DEBUGCON

[Gerd Hoffmann]

On 05/28/13 02:31, li guang wrote:
> 在 2013-05-27一的 11:13 +0200，Gerd Hoffmann写道：
>> On 05/24/13 04:47, liguang wrote:
>>> when enable DEBUG_DEBUGCON, there are some message
>>> printing bugs, so fix them.
>>
>> I'd suggest to either simply remove the debug printfs or turn them into
>> tracepoints.
>>
> 
> sorry, why we simply remove debug message printing?
> it's un-useful at all?

It's a case-by-case thing.

Debug messages logging guest activity like port access tend to be useful
for trouble-shooting.  They should be turned into tracepoints, so they
can be toggled at runtime and integrate nicely with tracing tools.

Sometimes debug messages are just leftovers and don't serve a real
purpose (any more).  Then it is best to simply remove them.

In the debugcon case I'd tend to simply remove them given how trivial
the device is.  But if you prefer making them tracepoints I'm ok with that.

cheers,
  Gerd