Re: [Qemu-devel] [PATCH v8 14/14] target: Use qemu_log() instead of fprintf(stderr, ...)

[Thomas Huth]

On 05.02.2018 19:07, Alistair Francis wrote:
> On Mon, Feb 5, 2018 at 6:31 AM, Markus Armbruster <address@hidden> wrote:
>> Thomas Huth <address@hidden> writes:
>>
>>> On 05.02.2018 07:33, Markus Armbruster wrote:
>>>> Thomas Huth <address@hidden> writes:
>>>>
>>>>> On 03.02.2018 09:43, Markus Armbruster wrote:
>>>>>> From: Alistair Francis <address@hidden>
>>>>>>
>>>>>> Convert fprintf(stderr, ...) to use qemu_log(). Double prints in
>>>>>> target/ppc/translate.c were manually remove. A fprintf() in
>>>>>> target/sh4/translate.c was kept as it's inside a #if 0. The #if 0 and
>>>>>> fflush() was removed around the unimplemented log in
>>>>>> target/sh4/translate.c as well.
>>>>>>
>>>>>> Signed-off-by: Alistair Francis <address@hidden>
>>>>>> [Trivial conflict with 6f1c2af641d resolved]
>>>>>> Signed-off-by: Markus Armbruster <address@hidden>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>>>>>> ---
>>>>>>  target/cris/translate.c      |  2 +-
>>>>>>  target/ppc/translate.c       | 36 ++++++++++--------------------------
>>>>>>  target/sh4/translate.c       |  7 ++-----
>>>>>>  target/unicore32/translate.c |  2 +-
>>>>>>  4 files changed, 14 insertions(+), 33 deletions(-)
>>>>>>
>>>>>> diff --git a/target/cris/translate.c b/target/cris/translate.c
>>>>>> index f51a731db9..ff31311ed0 100644
>>>>>> --- a/target/cris/translate.c
>>>>>> +++ b/target/cris/translate.c
>>>>>> @@ -137,7 +137,7 @@ typedef struct DisasContext {
>>>>>>
>>>>>>  static void gen_BUG(DisasContext *dc, const char *file, int line)
>>>>>>  {
>>>>>> -    fprintf(stderr, "BUG: pc=%x %s %d\n", dc->pc, file, line);
>>>>>> +    qemu_log("BUG: pc=%x %s %d\n", dc->pc, file, line);
>>>>>>      if (qemu_log_separate()) {
>>>>>>          qemu_log("BUG: pc=%x %s %d\n", dc->pc, file, line);
>>>>>>      }
>>>>>
>>>>> This one is still logging twice now.
>>>>
>>>> Hmm.
>>>>
>>>>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>>>>> index 4132f67bb1..172c9f2001 100644
>>>>>> --- a/target/ppc/translate.c
>>>>>> +++ b/target/ppc/translate.c
>>>>>> @@ -3933,12 +3933,8 @@ static inline void gen_op_mfspr(DisasContext *ctx)
>>>>>>               * allowing userland application to read the PVR
>>>>>>               */
>>>>>>              if (sprn != SPR_PVR) {
>>>>>> -                fprintf(stderr, "Trying to read privileged spr %d 
>>>>>> (0x%03x) at "
>>>>>> -                        TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
>>>>>> -                if (qemu_log_separate()) {
>>>>>> -                    qemu_log("Trying to read privileged spr %d (0x%03x) 
>>>>>> at "
>>>>>> -                             TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 
>>>>>> 4);
>>>>>> -                }
>>>>>> +                qemu_log("Trying to read privileged spr %d (0x%03x) at "
>>>>>> +                         TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4);
>>>>>
>>>>> I wonder whether that should maybe rather be a
>>>>> qemu_log_mask(LOG_GUEST_ERROR, ...) instead? Well, but maybe that's
>>>>> subject to another patch...
>>>>
>>>> qemu_log_separate() appears to be always used like this
>>>>
>>>>     fprintf(stderr, ... the message ...);
>>>>     if (qemu_log_separate()) {
>>>>         qemu_log(... the same message ...);
>>>>     }
>>>>
>>>> Are you proposing to replace this pattern by
>>>>
>>>>     qemu_log_mask(LOG_GUEST_ERROR, ...the message ...);
>>>>
>>>> ?
>>>
>>> Not globally, only in target/ppc/translate.c. The wrong accesses to SPR
>>> (special purpose registers) there indicate that the guest has likely
>>> tried to do something wrong.
>>>
>>> Globally, I wonder whether it still makes sense to keep the
>>> qemu_log_separate() stuff - we rather want to get rid of all fprintfs,
>>> don't we?
>>
>> The qemu_log_separate() pattern feels wrong to me.  If we have a class
>> of messages that should go to stderr in addition to the log (unless the
>> two are the same), then we should have a log level / mask / function /
>> whatever to do that.
>>
>> To expedite merging of the rest of the series, I'll drop this patch from
>> it.

Or maybe just drop the target/ppc parts?

>> Alistair, would you be willing to work with Thomas to revise this patch?
> 
> Yeah, I'm happy to.
> 
> What do we think then? Just swap to qemu_log_mask() and
> error_report()? Or do we need something more?

IMHO just switching to qemu_log* and error_report should be sufficient.
At least for the target/ppc parts. If we also want to replace the other
spots that use qemu_log_separate(), that might need some more
discussions, but at least I think we should try to do everything with
error_report() and qemu_log* directly, without introducing yet another
wrapper function for this.

 Thomas