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

[Alex Bennée]

Markus Armbruster <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> 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!
>
> Double-checking... you want me to keep goto reset_flash, like this:
>
> @@ -623,8 +617,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
>              } else {
> -                DPRINTF("%s: unknown command for \"write block\"\n", 
> __func__);
> -                PFLASH_BUG("Write block confirm");
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "unknown command for \"write block\"\n");
>                  goto reset_flash;
>              }
>              break;

With this change:

Reviewed-by: Alex Bennée <address@hidden>

--
Alex Bennée