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

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <eblake@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> 
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Acked-by: John Snow <jsnow@redhat.com>
>
> 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.

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