Re: [Qemu-devel] [PATCH 02/10] pflash: Macro PFLASH_BUG() is used just once, expand

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On Tue, 19 Feb 2019 at 12:41, Philippe Mathieu-Daudé <address@hidden> wrote:
>>
>> On 2/18/19 1:56 PM, Markus Armbruster wrote:
>> > PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
>> > BUG", which sounds like a warning, then calls exit(1), followed by
>> > unreachable goto reset_flash.  All this commit does is expanding the
>> > macro, so the smell becomes more poignant, and the macro can be
>> > deleted.
>> >
>> > Signed-off-by: Markus Armbruster <address@hidden>
>> > ---
>> >  hw/block/pflash_cfi01.c | 10 ++--------
>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> > index 9efa7aa9af..f73c30a3ee 100644
>> > --- a/hw/block/pflash_cfi01.c
>> > +++ b/hw/block/pflash_cfi01.c
>> > @@ -49,12 +49,6 @@
>> >  #include "sysemu/sysemu.h"
>> >  #include "trace.h"
>> >
>> > -#define PFLASH_BUG(fmt, ...) \
>> > -do { \
>> > -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
>> > -    exit(1); \
>> > -} while(0)
>> > -
>> >  /* #define PFLASH_DEBUG */
>> >  #ifdef PFLASH_DEBUG
>> >  #define DPRINTF(fmt, ...)                                   \
>> > @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr 
>> > offset,
>> >                  pfl->status |= 0x80;
>> >              } else {
>> >                  DPRINTF("%s: unknown command for \"write block\"\n", 
>> > __func__);
>> > -                PFLASH_BUG("Write block confirm");
>> > -                goto reset_flash;
>> > +                fprintf(stderr, "PFLASH: Possible BUG - Write block 
>> > confirm");
>> > +                exit(1);
>>
>> Don't you want to use hw_error here?
>>
>>       hw_error("PFLASH: Possible BUG - Write block confirm");
>
> This should just be
>    qemu_log_mask(LOG_GUEST_ERROR, ...);
> (replacing both the DPRINTF and the PFLASH_BUG()).
>
> It's triggerable by a guest (if it puts the device into write-block
> mode and then feeds it a bogus command byte), so it's just a guest
> error, not an issue with our model of the pflash.

I can do that.  Thanks!