[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups
From: |
Andrew Jones |
Subject: |
Re: [Qemu-devel] [PATCH] hw/dma/xilinx_axidma: debug printf fixups |
Date: |
Wed, 6 Jan 2016 13:51:07 +0100 |
User-agent: |
Mutt/1.5.23.1 (2014-03-12) |
On Tue, Jan 05, 2016 at 05:45:57PM -0800, Alistair Francis wrote:
> On Tue, Jan 5, 2016 at 7:32 AM, Andrew Jones <address@hidden> 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 <address@hidden>
> >> > ---
> >> > 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