Re: [Qemu-block] [Qemu-devel] [PATCH 1/4] hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()

[Philippe Mathieu-Daudé]

On 06/07/2018 10:41 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
>> On 05/30/2018 05:23 PM, John Snow wrote:
>>> On 05/29/2018 01:48 PM, Philippe Mathieu-Daudé wrote:
>>>> Use error_report() + abort() instead of error_setg(&error_abort),
>>>> as suggested by the "qapi/error.h" documentation:
>>>>
>>>>     Please don't error_setg(&error_fatal, ...), use error_report() and
>>>>     exit(), because that's more obvious.
>>>>     Likewise, don't error_setg(&error_abort, ...), use assert().
>>>>
>>>
>>> Didn't realize this was bad form (Why do we have it?)
> 
> Because &error_abort has other uses, and I can't see how we can get rid
> of just error_setg(&error_abort, ...).  Open to ideas.
> 
>>>> Use abort() instead of the suggested assert() because the error message
>>>> already got displayed.
>>>>
>>>
>>> Update the suggestion?
>>
>> Fair enough.
> 
> No.  I intentionally pointed to assert(), because ...
> 
>>>> Suggested-by: Eric Blake <address@hidden>
>>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>>
>>> Reviewed-by: John Snow <address@hidden>
>>> Acked-by: John Snow <address@hidden>
>>
>> Thanks!
>>
>>>> ---
>>>>  hw/block/fdc.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index cd29e27d8f..048467c00b 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -401,9 +401,10 @@ static int pick_geometry(FDrive *drv)
>>>>  
>>>>      /* No match of any kind found -- fd_format is misconfigured, abort. */
>>>>      if (match == -1) {
>>>> -        error_setg(&error_abort, "No candidate geometries present in 
>>>> table "
>>>> -                   " for floppy drive type '%s'",
>>>> -                   FloppyDriveType_str(drv->drive));
>>>> +        error_report("No candidate geometries present in table "
>>>> +                     " for floppy drive type '%s'",
>>>> +                     FloppyDriveType_str(drv->drive));
>>>> +        abort();
> 
> ... no matter how "nice" you make the message before an abort, it's
> still an abort.  It should not happen (or else aborting would be wrong),
> and when it does, somebody will have to stare at the code anyway.  I
> recommend not to bother with niceties there.

OK I see, thanks for the clear explanation.

> 
> Pig:
> 
>     hw/block/fdc.c:403:pick_geometry: assertion failed: (match != -1)
>     Aborted (core dumped)
> 
> Pig with lipstick on:
> 
>     No candidate geometries present in table for floppy drive type 'FOO'
>     Aborted (core dumped)
> 
> Now, if you're really, really into lipstick: to each his own.  But
> please return the courtesy and keep it out of the stuff I maintain :)
> 
>>>>      }
>>>>  
>>>>      parse = &(fd_formats[match]);
>>>> -- 
>>>> 2.17.0