| Message ID | 20200514192047.5297-18-aleksandar.qemu.devel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/mips: FPU and other cleanups and improvements | expand |
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> writes: > The checkpatch complain about "#ifdef 0". Convert corresponding > dead code to comments. In future, these cases could be converted > to some no-nonsense logging/tracing. > > Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> > CC: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/mips/mips_malta.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c > index e4c4de1b4e..f91fa34b06 100644 > --- a/hw/mips/mips_malta.c > +++ b/hw/mips/mips_malta.c > @@ -427,10 +427,12 @@ static uint64_t malta_fpga_read(void *opaque, hwaddr addr, > break; > > default: > -#if 0 > - printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n", > - addr); > -#endif > +/* > + * Possible logging: > + * > + * printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n", > + * addr); > + */ > break; > } > return val; > @@ -515,10 +517,12 @@ static void malta_fpga_write(void *opaque, hwaddr addr, > break; > > default: > -#if 0 > - printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n", > - addr); > -#endif > +/* > + * Possible logging: > + * > + * printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n", > + * addr); > + */ > break; > } > } Please don't. Checkpatch warns "if this code is redundant consider removing it\n". If it is redundant, do remove it. If it is not redundant, do ignore checkpatch's warning, do not abuse comments to hide from checkpatch. We'd rather not have to code up a warning for that :) These two look like they want to be tracepoints.
пет, 15. мај 2020. у 09:53 Markus Armbruster <armbru@redhat.com> је написао/ла: > > Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> writes: > > > The checkpatch complain about "#ifdef 0". Convert corresponding > > dead code to comments. In future, these cases could be converted > > to some no-nonsense logging/tracing. > > > > Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> > > CC: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > hw/mips/mips_malta.c | 20 ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c > > index e4c4de1b4e..f91fa34b06 100644 > > --- a/hw/mips/mips_malta.c > > +++ b/hw/mips/mips_malta.c > > @@ -427,10 +427,12 @@ static uint64_t malta_fpga_read(void *opaque, hwaddr addr, > > break; > > > > default: > > -#if 0 > > - printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n", > > - addr); > > -#endif > > +/* > > + * Possible logging: > > + * > > + * printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n", > > + * addr); > > + */ > > break; > > } > > return val; > > @@ -515,10 +517,12 @@ static void malta_fpga_write(void *opaque, hwaddr addr, > > break; > > > > default: > > -#if 0 > > - printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n", > > - addr); > > -#endif > > +/* > > + * Possible logging: > > + * > > + * printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n", > > + * addr); > > + */ > > break; > > } > > } > > Please don't. > > Checkpatch warns "if this code is redundant consider removing it\n". > > If it is redundant, do remove it. > > If it is not redundant, do ignore checkpatch's warning, do not abuse > comments to hide from checkpatch. We'd rather not have to code up a > warning for that :) > > These two look like they want to be tracepoints. > Hi, Markus. I understood your points. They make sense to me. In hindsight, in general, we shouldn't try just to silence checkpatch warnings (or, for that matter, compiler warnings as well), but try to resolve the root cause, the underlying issue, of the warning. In this case, creating tracepoints seems to be the right thing to do. In hindsight too, this was my "quick and dirty" way of getting rid of two checkpatch warnings. Thanks for your remarks! Aleksandar P. S. The ultimate reason for this patch is that we plan renaming this file in near future, and want to do it in "checkpatch-warning-free" manner.
On Fri, 15 May 2020 at 12:07, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> wrote: > I understood your points. They make sense to me. In hindsight, in > general, we shouldn't try just to silence checkpatch warnings (or, for > that matter, compiler warnings as well), but try to resolve the root > cause, the underlying issue, of the warning. In this case, creating > tracepoints seems to be the right thing to do. For this sort of "default case, guest accessed a bad register offset" case, what I usually do is something like qemu_log_mask(LOG_GUEST_ERROR, "malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n", addr); That's a simpler change than adding tracepoints and matches how we report this kind of guest-did-the-wrong-thing behaviour elsewhere. thanks -- PMM
пет, 15. мај 2020. у 13:12 Peter Maydell <peter.maydell@linaro.org> је написао/ла: > > On Fri, 15 May 2020 at 12:07, Aleksandar Markovic > <aleksandar.qemu.devel@gmail.com> wrote: > > I understood your points. They make sense to me. In hindsight, in > > general, we shouldn't try just to silence checkpatch warnings (or, for > > that matter, compiler warnings as well), but try to resolve the root > > cause, the underlying issue, of the warning. In this case, creating > > tracepoints seems to be the right thing to do. > > For this sort of "default case, guest accessed a bad register offset" > case, what I usually do is something like > qemu_log_mask(LOG_GUEST_ERROR, "malta_fpga_write: Bad register offset 0x" > TARGET_FMT_lx "\n", addr); > > That's a simpler change than adding tracepoints and matches how > we report this kind of guest-did-the-wrong-thing behaviour elsewhere. > Oh, great! I appreciate your remark and guidance very much! I am going to correct this patch in v3. Sincerely, Aleksandar > thanks > -- PMM
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c index e4c4de1b4e..f91fa34b06 100644 --- a/hw/mips/mips_malta.c +++ b/hw/mips/mips_malta.c @@ -427,10 +427,12 @@ static uint64_t malta_fpga_read(void *opaque, hwaddr addr, break; default: -#if 0 - printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n", - addr); -#endif +/* + * Possible logging: + * + * printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n", + * addr); + */ break; } return val; @@ -515,10 +517,12 @@ static void malta_fpga_write(void *opaque, hwaddr addr, break; default: -#if 0 - printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n", - addr); -#endif +/* + * Possible logging: + * + * printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n", + * addr); + */ break; } }
The checkpatch complain about "#ifdef 0". Convert corresponding dead code to comments. In future, these cases could be converted to some no-nonsense logging/tracing. Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> CC: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/mips/mips_malta.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)