On 06/07/2018 10:41 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4...@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 <ebl...@redhat.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>>>
>>> Reviewed-by: John Snow <js...@redhat.com>
>>> Acked-by: John Snow <js...@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.
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
