| Message ID | 1452000144-23805-1-git-send-email-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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 >> > > >
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
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
(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(-)