On 05.02.2018 19:07, Alistair Francis wrote:
> On Mon, Feb 5, 2018 at 6:31 AM, Markus Armbruster <arm...@redhat.com> wrote:
>> Thomas Huth <th...@redhat.com> writes:
>>
>>> On 05.02.2018 07:33, Markus Armbruster wrote:
>>>> Thomas Huth <th...@redhat.com> writes:
>>>>
>>>>> On 03.02.2018 09:43, Markus Armbruster wrote:
>>>>>> From: Alistair Francis <alistair.fran...@xilinx.com>
>>>>>>
>>>>>> 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 <alistair.fran...@xilinx.com>
>>>>>> [Trivial conflict with 6f1c2af641d resolved]
>>>>>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>>>>>> ---
>>>>>> 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
