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

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 02/18/19 13:56, 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);
>>              }
>>              break;
>>          default:
>> 
>
> Technically speaking, the commit message is slightly incorrect, where it
> says "all this commit does is expanding the macro" -- the "goto" is
> being removed as well.

You're right.  I'll amend the commit message.

> I like the attention to detail in that you didn't add the missing
> newline character in the expanded fprintf() ;)

I wish I could claim it was intentional preservation of a bad smell as
fair warning to future readers...

> With the commit message tweaked, or not:
>
> Reviewed-by: Laszlo Ersek <address@hidden>

Thanks!