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

[Markus Armbruster]

Markus Armbruster <arm...@redhat.com> writes:

> Peter Maydell <peter.mayd...@linaro.org> writes:
>
>> On Tue, 19 Feb 2019 at 12:41, Philippe Mathieu-Daudé <phi...@redhat.com> 
>> 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 <arm...@redhat.com>
>>> > ---
>>> >  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;