| Message ID | 1337074749-24189-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
Am 15.05.2012 11:39, schrieb Fabien Chouteau: > Do not call cpu_dump_state if logfile is NULL. And where is log_cpu_state() being called from? Its caller is passing NULL already then. Andreas > > Signed-off-by: Fabien Chouteau <chouteau@adacore.com> > --- > qemu-log.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/qemu-log.h b/qemu-log.h > index fccfb110..2cd5ffa 100644 > --- a/qemu-log.h > +++ b/qemu-log.h > @@ -51,7 +51,12 @@ extern int loglevel; > /* Special cases: */ > > /* cpu_dump_state() logging functions: */ > -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f)); > +#define log_cpu_state(env, f) \ > +do { \ > + if (logfile != NULL) { \ > + cpu_dump_state((env), logfile, fprintf, (f)); \ > + } \ > + } while (0) > #define log_cpu_state_mask(b, env, f) do { \ > if (loglevel & (b)) log_cpu_state((env), (f)); \ > } while (0)
On 05/15/2012 03:31 PM, Andreas Färber wrote: > Am 15.05.2012 11:39, schrieb Fabien Chouteau: >> Do not call cpu_dump_state if logfile is NULL. > > And where is log_cpu_state() being called from? Its caller is passing > NULL already then. > No, logfile is a global variable. log_cpu_state() takes only CPUState and flags parameters. >> >> Signed-off-by: Fabien Chouteau <chouteau@adacore.com> >> --- >> qemu-log.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-log.h b/qemu-log.h >> index fccfb110..2cd5ffa 100644 >> --- a/qemu-log.h >> +++ b/qemu-log.h >> @@ -51,7 +51,12 @@ extern int loglevel; >> /* Special cases: */ >> >> /* cpu_dump_state() logging functions: */ >> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f)); >> +#define log_cpu_state(env, f) \ >> +do { \ >> + if (logfile != NULL) { \ >> + cpu_dump_state((env), logfile, fprintf, (f)); \ >> + } \ >> + } while (0) >> #define log_cpu_state_mask(b, env, f) do { \ >> if (loglevel & (b)) log_cpu_state((env), (f)); \ >> } while (0) >
On 15 May 2012 17:08, Fabien Chouteau <chouteau@adacore.com> wrote: > On 05/15/2012 03:31 PM, Andreas Färber wrote: >> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>> Do not call cpu_dump_state if logfile is NULL. >> >> And where is log_cpu_state() being called from? Its caller is passing >> NULL already then. > No, logfile is a global variable. log_cpu_state() takes only CPUState > and flags parameters. The question is which of the following two options we want: (1) callers should be guarding the calls to log_cpu_state() with checks for qemu_log_enabled() or qemu_loglevel_mask() (2) log_cpu_state() does its own check for whether logging is enabled in the same way that qemu_log() and qemu_log_vprintf() do At the moment most callers of log_cpu_state() do their own checks as per (1), but you could make an argument that we should switch to (2) instead. -- PMM
On 05/15/2012 06:20 PM, Peter Maydell wrote: > On 15 May 2012 17:08, Fabien Chouteau <chouteau@adacore.com> wrote: >> On 05/15/2012 03:31 PM, Andreas Färber wrote: >>> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>>> Do not call cpu_dump_state if logfile is NULL. >>> >>> And where is log_cpu_state() being called from? Its caller is passing >>> NULL already then. > >> No, logfile is a global variable. log_cpu_state() takes only CPUState >> and flags parameters. > > The question is which of the following two options we want: > (1) callers should be guarding the calls to log_cpu_state() with > checks for qemu_log_enabled() or qemu_loglevel_mask() > (2) log_cpu_state() does its own check for whether logging is enabled > in the same way that qemu_log() and qemu_log_vprintf() do > > At the moment most callers of log_cpu_state() do their own checks > as per (1), but you could make an argument that we should switch > to (2) instead. > I think (2) is better, we do the check in one place and that's it. All call to log_cpu_state are safe. And as you said, it's already the way qemu_log and qemu_log_vprintf work.
On 15 May 2012 17:45, Fabien Chouteau <chouteau@adacore.com> wrote: > On 05/15/2012 06:20 PM, Peter Maydell wrote: >> The question is which of the following two options we want: >> (1) callers should be guarding the calls to log_cpu_state() with >> checks for qemu_log_enabled() or qemu_loglevel_mask() >> (2) log_cpu_state() does its own check for whether logging is enabled >> in the same way that qemu_log() and qemu_log_vprintf() do >> >> At the moment most callers of log_cpu_state() do their own checks >> as per (1), but you could make an argument that we should switch >> to (2) instead. > > I think (2) is better, we do the check in one place and that's it. All > call to log_cpu_state are safe. And as you said, it's already the way > qemu_log and qemu_log_vprintf work. Yeah, it seems reasonable to me. I think your commit message could be better though, since you're actually kind of changing an API here, not just fixing a segfault bug. -- PMM
Am 15.05.2012 18:08, schrieb Fabien Chouteau: > On 05/15/2012 03:31 PM, Andreas Färber wrote: >> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>> Do not call cpu_dump_state if logfile is NULL. >> >> And where is log_cpu_state() being called from? Its caller is passing >> NULL already then. >> > > No, logfile is a global variable. log_cpu_state() takes only CPUState > and flags parameters. Ah, I see now that f is a different f here, logfile becomes log_cpu_state()'s f. Unfortunate naming. Your fix looks OK then but I would recommend turning it into a static inline function to avoid the line breaks. Andreas >>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com> >>> --- >>> qemu-log.h | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/qemu-log.h b/qemu-log.h >>> index fccfb110..2cd5ffa 100644 >>> --- a/qemu-log.h >>> +++ b/qemu-log.h >>> @@ -51,7 +51,12 @@ extern int loglevel; >>> /* Special cases: */ >>> >>> /* cpu_dump_state() logging functions: */ >>> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f)); >>> +#define log_cpu_state(env, f) \ >>> +do { \ >>> + if (logfile != NULL) { \ >>> + cpu_dump_state((env), logfile, fprintf, (f)); \ >>> + } \ >>> + } while (0) >>> #define log_cpu_state_mask(b, env, f) do { \ >>> if (loglevel & (b)) log_cpu_state((env), (f)); \ >>> } while (0)
On 05/16/2012 05:50 AM, Andreas Färber wrote: > Am 15.05.2012 18:08, schrieb Fabien Chouteau: >> On 05/15/2012 03:31 PM, Andreas Färber wrote: >>> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>>> Do not call cpu_dump_state if logfile is NULL. >>> >>> And where is log_cpu_state() being called from? Its caller is passing >>> NULL already then. >>> >> >> No, logfile is a global variable. log_cpu_state() takes only CPUState >> and flags parameters. > > Ah, I see now that f is a different f here, logfile becomes > log_cpu_state()'s f. Unfortunate naming. > > Your fix looks OK then but I would recommend turning it into a static > inline function to avoid the line breaks. > In this case I can rewrite all the macros in qemu-log.h to static inline.
On 05/16/2012 10:29 AM, Fabien Chouteau wrote: > On 05/16/2012 05:50 AM, Andreas Färber wrote: >> Am 15.05.2012 18:08, schrieb Fabien Chouteau: >>> On 05/15/2012 03:31 PM, Andreas Färber wrote: >>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>>>> Do not call cpu_dump_state if logfile is NULL. >>>> >>>> And where is log_cpu_state() being called from? Its caller is passing >>>> NULL already then. >>>> >>> >>> No, logfile is a global variable. log_cpu_state() takes only CPUState >>> and flags parameters. >> >> Ah, I see now that f is a different f here, logfile becomes >> log_cpu_state()'s f. Unfortunate naming. >> >> Your fix looks OK then but I would recommend turning it into a static >> inline function to avoid the line breaks. >> > > In this case I can rewrite all the macros in qemu-log.h to static inline. > This is more complex than expected... 1 - GCC rejects inlined variadic functions 2 - Moving from macro to inline implies use of types defined in cpu.h (target_ulong, CPUArchState...), which I cannot include because qemu-log.h is used in tools (i.e. without cpu.h). Conclusion: unless someone volunteer for a massive restructuring of qemu-log we have to keep the marcro for log_cpu_state.
On 05/16/2012 03:39 PM, Fabien Chouteau wrote: > On 05/16/2012 10:29 AM, Fabien Chouteau wrote: >> On 05/16/2012 05:50 AM, Andreas Färber wrote: >>> Am 15.05.2012 18:08, schrieb Fabien Chouteau: >>>> On 05/15/2012 03:31 PM, Andreas Färber wrote: >>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>>>>> Do not call cpu_dump_state if logfile is NULL. >>>>> >>>>> And where is log_cpu_state() being called from? Its caller is passing >>>>> NULL already then. >>>>> >>>> >>>> No, logfile is a global variable. log_cpu_state() takes only CPUState >>>> and flags parameters. >>> >>> Ah, I see now that f is a different f here, logfile becomes >>> log_cpu_state()'s f. Unfortunate naming. >>> >>> Your fix looks OK then but I would recommend turning it into a static >>> inline function to avoid the line breaks. >>> >> >> In this case I can rewrite all the macros in qemu-log.h to static inline. >> > > This is more complex than expected... > > 1 - GCC rejects inlined variadic functions > > 2 - Moving from macro to inline implies use of types defined in cpu.h > (target_ulong, CPUArchState...), which I cannot include because > qemu-log.h is used in tools (i.e. without cpu.h). > > Conclusion: unless someone volunteer for a massive restructuring of > qemu-log we have to keep the marcro for log_cpu_state. > So, are we good with the second patch?
On 23.05.2012, at 17:43, Fabien Chouteau wrote: > On 05/16/2012 03:39 PM, Fabien Chouteau wrote: >> On 05/16/2012 10:29 AM, Fabien Chouteau wrote: >>> On 05/16/2012 05:50 AM, Andreas Färber wrote: >>>> Am 15.05.2012 18:08, schrieb Fabien Chouteau: >>>>> On 05/15/2012 03:31 PM, Andreas Färber wrote: >>>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>>>>>> Do not call cpu_dump_state if logfile is NULL. >>>>>> >>>>>> And where is log_cpu_state() being called from? Its caller is passing >>>>>> NULL already then. >>>>>> >>>>> >>>>> No, logfile is a global variable. log_cpu_state() takes only CPUState >>>>> and flags parameters. >>>> >>>> Ah, I see now that f is a different f here, logfile becomes >>>> log_cpu_state()'s f. Unfortunate naming. >>>> >>>> Your fix looks OK then but I would recommend turning it into a static >>>> inline function to avoid the line breaks. >>>> >>> >>> In this case I can rewrite all the macros in qemu-log.h to static inline. >>> >> >> This is more complex than expected... >> >> 1 - GCC rejects inlined variadic functions >> >> 2 - Moving from macro to inline implies use of types defined in cpu.h >> (target_ulong, CPUArchState...), which I cannot include because >> qemu-log.h is used in tools (i.e. without cpu.h). >> >> Conclusion: unless someone volunteer for a massive restructuring of >> qemu-log we have to keep the marcro for log_cpu_state. >> > > So, are we good with the second patch? No reply, so I assume that's a yes. Applied it to ppc-next :). Thanks a lot! Alex
On 05/30/2012 09:58 AM, Alexander Graf wrote: > > On 23.05.2012, at 17:43, Fabien Chouteau wrote: > >> On 05/16/2012 03:39 PM, Fabien Chouteau wrote: >>> On 05/16/2012 10:29 AM, Fabien Chouteau wrote: >>>> On 05/16/2012 05:50 AM, Andreas Färber wrote: >>>>> Am 15.05.2012 18:08, schrieb Fabien Chouteau: >>>>>> On 05/15/2012 03:31 PM, Andreas Färber wrote: >>>>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>>>>>>> Do not call cpu_dump_state if logfile is NULL. >>>>>>> >>>>>>> And where is log_cpu_state() being called from? Its caller is passing >>>>>>> NULL already then. >>>>>>> >>>>>> >>>>>> No, logfile is a global variable. log_cpu_state() takes only CPUState >>>>>> and flags parameters. >>>>> >>>>> Ah, I see now that f is a different f here, logfile becomes >>>>> log_cpu_state()'s f. Unfortunate naming. >>>>> >>>>> Your fix looks OK then but I would recommend turning it into a static >>>>> inline function to avoid the line breaks. >>>>> >>>> >>>> In this case I can rewrite all the macros in qemu-log.h to static inline. >>>> >>> >>> This is more complex than expected... >>> >>> 1 - GCC rejects inlined variadic functions >>> >>> 2 - Moving from macro to inline implies use of types defined in cpu.h >>> (target_ulong, CPUArchState...), which I cannot include because >>> qemu-log.h is used in tools (i.e. without cpu.h). >>> >>> Conclusion: unless someone volunteer for a massive restructuring of >>> qemu-log we have to keep the marcro for log_cpu_state. >>> >> >> So, are we good with the second patch? > > No reply, so I assume that's a yes. Applied it to ppc-next :). Thanks a lot! > You're welcome! Thanks,
Am 30.05.2012 09:58, schrieb Alexander Graf: > > On 23.05.2012, at 17:43, Fabien Chouteau wrote: > >> On 05/16/2012 03:39 PM, Fabien Chouteau wrote: >>> On 05/16/2012 10:29 AM, Fabien Chouteau wrote: >>>> On 05/16/2012 05:50 AM, Andreas Färber wrote: >>>>> Am 15.05.2012 18:08, schrieb Fabien Chouteau: >>>>>> On 05/15/2012 03:31 PM, Andreas Färber wrote: >>>>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau: >>>>>>>> Do not call cpu_dump_state if logfile is NULL. >>>>>>> >>>>>>> And where is log_cpu_state() being called from? Its caller is passing >>>>>>> NULL already then. >>>>>>> >>>>>> >>>>>> No, logfile is a global variable. log_cpu_state() takes only CPUState >>>>>> and flags parameters. >>>>> >>>>> Ah, I see now that f is a different f here, logfile becomes >>>>> log_cpu_state()'s f. Unfortunate naming. >>>>> >>>>> Your fix looks OK then but I would recommend turning it into a static >>>>> inline function to avoid the line breaks. >>>>> >>>> >>>> In this case I can rewrite all the macros in qemu-log.h to static inline. >>>> >>> >>> This is more complex than expected... >>> >>> 1 - GCC rejects inlined variadic functions >>> >>> 2 - Moving from macro to inline implies use of types defined in cpu.h >>> (target_ulong, CPUArchState...), which I cannot include because >>> qemu-log.h is used in tools (i.e. without cpu.h). >>> >>> Conclusion: unless someone volunteer for a massive restructuring of >>> qemu-log we have to keep the marcro for log_cpu_state. >>> >> >> So, are we good with the second patch? > > No reply, so I assume that's a yes. I'm okay with it if there's no better way. Didn't find the time to investigate myself. Andreas > Applied it to ppc-next :). Thanks a lot! > > > Alex
diff --git a/qemu-log.h b/qemu-log.h index fccfb110..2cd5ffa 100644 --- a/qemu-log.h +++ b/qemu-log.h @@ -51,7 +51,12 @@ extern int loglevel; /* Special cases: */ /* cpu_dump_state() logging functions: */ -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f)); +#define log_cpu_state(env, f) \ +do { \ + if (logfile != NULL) { \ + cpu_dump_state((env), logfile, fprintf, (f)); \ + } \ + } while (0) #define log_cpu_state_mask(b, env, f) do { \ if (loglevel & (b)) log_cpu_state((env), (f)); \ } while (0)
Do not call cpu_dump_state if logfile is NULL. Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- qemu-log.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)