[Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups

[Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups

[Andrew Jones]

(Found by grepping for broken PRI users.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/dma/xilinx_axidma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index b1cfa11356a26..2ab0772cd19ae 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -180,10 +180,10 @@ static inline int streamid_from_addr(hwaddr addr)
 #ifdef DEBUG_ENET
 static void stream_desc_show(struct SDesc *d)
 {
-    qemu_log("buffer_addr  = " PRIx64 "\n", d->buffer_address);
-    qemu_log("nxtdesc      = " PRIx64 "\n", d->nxtdesc);
-    qemu_log("control      = %x\n", d->control);
-    qemu_log("status       = %x\n", d->status);
+    qemu_log("buffer_addr  = 0x%" PRIx64 "\n", d->buffer_address);
+    qemu_log("nxtdesc      = 0x%" PRIx64 "\n", d->nxtdesc);
+    qemu_log("control      = 0x%x\n", d->control);
+    qemu_log("status       = 0x%x\n", d->status);
 }
 #endif
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On 01/05/2016 06:22 AM, Andrew Jones wrote:
> (Found by grepping for broken PRI users.)
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/dma/xilinx_axidma.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index b1cfa11356a26..2ab0772cd19ae 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -180,10 +180,10 @@ static inline int streamid_from_addr(hwaddr addr)
>  #ifdef DEBUG_ENET
>  static void stream_desc_show(struct SDesc *d)
>  {
> -    qemu_log("buffer_addr  = " PRIx64 "\n", d->buffer_address);
> -    qemu_log("nxtdesc      = " PRIx64 "\n", d->nxtdesc);
> -    qemu_log("control      = %x\n", d->control);
> -    qemu_log("status       = %x\n", d->status);
> +    qemu_log("buffer_addr  = 0x%" PRIx64 "\n", d->buffer_address);
> +    qemu_log("nxtdesc      = 0x%" PRIx64 "\n", d->nxtdesc);
> +    qemu_log("control      = 0x%x\n", d->control);
> +    qemu_log("status       = 0x%x\n", d->status);

This is dead code.  Nothing uses stream_desc_show() even when DEBUG_ENET
is defined.  I'd just delete the function and #ifdef altogether, instead.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups

[Andrew Jones]

On Tue, Jan 05, 2016 at 07:07:22AM -0700, Eric Blake wrote:
> On 01/05/2016 06:22 AM, Andrew Jones wrote:
> > (Found by grepping for broken PRI users.)
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  hw/dma/xilinx_axidma.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> > index b1cfa11356a26..2ab0772cd19ae 100644
> > --- a/hw/dma/xilinx_axidma.c
> > +++ b/hw/dma/xilinx_axidma.c
> > @@ -180,10 +180,10 @@ static inline int streamid_from_addr(hwaddr addr)
> >  #ifdef DEBUG_ENET
> >  static void stream_desc_show(struct SDesc *d)
> >  {
> > -    qemu_log("buffer_addr  = " PRIx64 "\n", d->buffer_address);
> > -    qemu_log("nxtdesc      = " PRIx64 "\n", d->nxtdesc);
> > -    qemu_log("control      = %x\n", d->control);
> > -    qemu_log("status       = %x\n", d->status);
> > +    qemu_log("buffer_addr  = 0x%" PRIx64 "\n", d->buffer_address);
> > +    qemu_log("nxtdesc      = 0x%" PRIx64 "\n", d->nxtdesc);
> > +    qemu_log("control      = 0x%x\n", d->control);
> > +    qemu_log("status       = 0x%x\n", d->status);
> 
> This is dead code.  Nothing uses stream_desc_show() even when DEBUG_ENET
> is defined.  I'd just delete the function and #ifdef altogether, instead.

Sounds good, but I guess I'll leave the keep+fix vs. throw decision to the
maintainers, rather than to submit a v2 ripping it out.

Thanks,
drew

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups

[Alistair Francis]

On Tue, Jan 5, 2016 at 7:32 AM, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Jan 05, 2016 at 07:07:22AM -0700, Eric Blake wrote:
>> On 01/05/2016 06:22 AM, Andrew Jones wrote:
>> > (Found by grepping for broken PRI users.)
>> >
>> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>> > ---
>> >  hw/dma/xilinx_axidma.c | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> > index b1cfa11356a26..2ab0772cd19ae 100644
>> > --- a/hw/dma/xilinx_axidma.c
>> > +++ b/hw/dma/xilinx_axidma.c
>> > @@ -180,10 +180,10 @@ static inline int streamid_from_addr(hwaddr addr)
>> >  #ifdef DEBUG_ENET
>> >  static void stream_desc_show(struct SDesc *d)
>> >  {
>> > -    qemu_log("buffer_addr  = " PRIx64 "\n", d->buffer_address);
>> > -    qemu_log("nxtdesc      = " PRIx64 "\n", d->nxtdesc);
>> > -    qemu_log("control      = %x\n", d->control);
>> > -    qemu_log("status       = %x\n", d->status);
>> > +    qemu_log("buffer_addr  = 0x%" PRIx64 "\n", d->buffer_address);
>> > +    qemu_log("nxtdesc      = 0x%" PRIx64 "\n", d->nxtdesc);
>> > +    qemu_log("control      = 0x%x\n", d->control);
>> > +    qemu_log("status       = 0x%x\n", d->status);
>>
>> This is dead code.  Nothing uses stream_desc_show() even when DEBUG_ENET
>> is defined.  I'd just delete the function and #ifdef altogether, instead.
>
> Sounds good, but I guess I'll leave the keep+fix vs. throw decision to the
> maintainers, rather than to submit a v2 ripping it out.

I don't see any reason to keep dead code around. I think it should be removed.

If you send a V2 removing it (or a new patch altogether) I'll review it.

Thanks,

Alistair

>
> Thanks,
> drew
>
>>
>> --
>> Eric Blake   eblake redhat com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>>
>
>
>

Re: [Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups

[Andrew Jones]

On Tue, Jan 05, 2016 at 05:45:57PM -0800, Alistair Francis wrote:
> On Tue, Jan 5, 2016 at 7:32 AM, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Jan 05, 2016 at 07:07:22AM -0700, Eric Blake wrote:
> >> On 01/05/2016 06:22 AM, Andrew Jones wrote:
> >> > (Found by grepping for broken PRI users.)
> >> >
> >> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> > ---
> >> >  hw/dma/xilinx_axidma.c | 8 ++++----
> >> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> >> > index b1cfa11356a26..2ab0772cd19ae 100644
> >> > --- a/hw/dma/xilinx_axidma.c
> >> > +++ b/hw/dma/xilinx_axidma.c
> >> > @@ -180,10 +180,10 @@ static inline int streamid_from_addr(hwaddr addr)
> >> >  #ifdef DEBUG_ENET
> >> >  static void stream_desc_show(struct SDesc *d)
> >> >  {
> >> > -    qemu_log("buffer_addr  = " PRIx64 "\n", d->buffer_address);
> >> > -    qemu_log("nxtdesc      = " PRIx64 "\n", d->nxtdesc);
> >> > -    qemu_log("control      = %x\n", d->control);
> >> > -    qemu_log("status       = %x\n", d->status);
> >> > +    qemu_log("buffer_addr  = 0x%" PRIx64 "\n", d->buffer_address);
> >> > +    qemu_log("nxtdesc      = 0x%" PRIx64 "\n", d->nxtdesc);
> >> > +    qemu_log("control      = 0x%x\n", d->control);
> >> > +    qemu_log("status       = 0x%x\n", d->status);
> >>
> >> This is dead code.  Nothing uses stream_desc_show() even when DEBUG_ENET
> >> is defined.  I'd just delete the function and #ifdef altogether, instead.
> >
> > Sounds good, but I guess I'll leave the keep+fix vs. throw decision to the
> > maintainers, rather than to submit a v2 ripping it out.
> 
> I don't see any reason to keep dead code around. I think it should be removed.

The reason I see, is that this function could be useful to temporarily add it
different places while debugging. I.e. this is no different than a collection
of temporary printf's, but saves the time of rewriting those printf's
whenever/wherever they're necessary. I suspect that's why this function is
here in the first place.

That said, I don't debug this file, so I don't really have any say on
whether or not it's of any use now. Anyway, based on the fact this function
has PRI bugs in it that would break compilation, I guess nobody debugs this
file with DEBUG_ENET turned on.

> 
> If you send a V2 removing it (or a new patch altogether) I'll review it.

OK, sending a new patch that kills it.

Thanks,
drew