qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH v6 04/29] hw/arm: Replace fprintf


From: Alistair Francis
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH v6 04/29] hw/arm: Replace fprintf(stderr, "*\n" with error_report()
Date: Mon, 8 Jan 2018 16:32:51 -0800

On Tue, Jan 2, 2018 at 4:59 AM, Markus Armbruster <address@hidden> wrote:
> Alistair Francis <address@hidden> writes:
>
>> On Fri, Dec 22, 2017 at 12:30 PM, Markus Armbruster <address@hidden> wrote:
>>> Alistair Francis <address@hidden> writes:
>>>
>>>> On Fri, Dec 22, 2017 at 9:17 AM, Thomas Huth <address@hidden> wrote:
>>>>> On 22.12.2017 16:37, Markus Armbruster wrote:
>>>>>> Second thoughts...
>>>>>>
>>>>>> Alistair Francis <address@hidden> writes:
>>>>> [...]
>>>>>>>  #include "qemu/osdep.h"
>>>>>>> +#include "qemu/error-report.h"
>>>>>>>  #include "qapi/error.h"
>>>>>>>  #include "qemu-common.h"
>>>>>>>  #include "cpu.h"
>>>>>>> @@ -1311,8 +1312,8 @@ static void omap_prcm_apll_update(struct 
>>>>>>> omap_prcm_s *s)
>>>>>>>      /* TODO: update clocks */
>>>>>>>
>>>>>>>      if (mode[0] == 1 || mode[0] == 2 || mode[1] == 1 || mode[1] == 2)
>>>>>>> -        fprintf(stderr, "%s: bad EN_54M_PLL or bad EN_96M_PLL\n",
>>>>>>> -                        __func__);
>>>>>>> +        error_report("%s: bad EN_54M_PLL or bad EN_96M_PLL",
>>>>>>> +                     __func__);
>>>>>>>  }
>>>>>>
>>>>>> This one's different: we neither exit() nor return a "failed" status to
>>>>>> the caller.
>>>>>>
>>>>>> We get here when the guest writes something funny to a certain
>>>>>> memory-mapped I/O register.  In other words, it's guest misbehavior, not
>>>>>> a user error.  I doubt it should be reported with error_report().
>>>>>> Peter, do we have a canonical way to report or log  guest misbehavior?
>>>>>
>>>>> qemu_log_mask(LOG_GUEST_ERROR, ...) ?
>>>>
>>>> That seems like the best option to me.
>>>
>>> Suggest:
>>>
>>> 1. Keep converting fatal errors (the ones that exit())
>>>
>>> 2. Keep converting recoverable errors (the ones that return failure)
>>>
>>> 3. You can leave the prints that are neither alone.  You can also
>>>    convert to logging or tracing, as appropriate, but that requires
>>>    understanding the code.
>>>
>>> Makes sense?
>>
>> Does this apply to new patches after this series or to this series as
>> well? The series is mostly just mechanical find/replace. I really
>> don't want to have to dig through every patch to figure out what to
>> change/not change.
>
> I understand your reluctance to sort patch hunks into buckets 1., 2. and
> 3. manually: there's an awful lot of hunks to sort.
>
> We know we have many fprintf() that should be error_report(),
> error_setg(), logging or tracing.
>
> We know we have error_report() that should be error_setg().
>
> Converting fprintf() to error_report() where we should really use
> something else makes the situation worse, I'm afraid.
>
> Since we need to sort, and sorting manually isn't practical, we need to
> automate.
>
> The patterns to recognize are 1. fprintf() followed by exit() and
> 2. fprintf() followed by return failure.
>
> Recognizing the patterns when there's stuff between fprintf() and exit()
> / return may exceed sed's power.  Feels like a Coccinelle job to me.
> Let's focus on the common case where exit() / return follows fprintf()
> immediately.
>
> Let's start with the easiest case: exit().  I figure that's still in
> reach of your find + sed tooling.
>
> Recognizing "return failure" is slightly harder, because error values
> aren't always obvious.  Common ones are return NULL, return -1, return
> -EFOO.
>
> I hope that peeling off truly simple cases like this will reduce the
> remaining hunks sufficiently to permit manual review.  If it doesn't, we
> should still get a major part of your work without making the situation
> worse.

Ok. So it is becoming apparent that this series is not going to be
accepted. I might just drop it and try and re-group with Coccinelle at
some point in the future.

If there is a subset of this series that is fine to go in I'm happy to
rebase and keep going, but at the moment it looks like starting again
is gong to be the least painful effort.

Alistair

>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]