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

[Markus Armbruster]

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.

Alistair, would you be willing to work with Thomas to revise this patch?