Message ID | 20180607144645.10187-2-f4bug@amsat.org |
---|---|
State | Superseded, archived |
Headers | show |
Series | qapi/error: converts error_setg(&error_fatal) to error_report() + exit() | expand |
On 06/07/2018 10:46 AM, Philippe Mathieu-Daudé wrote: > Use assert() 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(). > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/block/fdc.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index cd29e27d8f..7c1c57f57f 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv) > nb_sectors, > FloppyDriveType_str(parse->drive)); > } > + assert(type_match != -1 && "misconfigured fd_format"); > match = type_match; > } > - > - /* 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)); > - } > - > parse = &(fd_formats[match]); > > out: > Truthfully sad to see the lipstick go, because this is my pig. Oh well, it doesn't matter. Not really, anyway. ACK
John Snow <jsnow@redhat.com> writes: > On 06/07/2018 10:46 AM, Philippe Mathieu-Daudé wrote: >> Use assert() 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(). >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/block/fdc.c | 9 +-------- >> 1 file changed, 1 insertion(+), 8 deletions(-) >> >> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >> index cd29e27d8f..7c1c57f57f 100644 >> --- a/hw/block/fdc.c >> +++ b/hw/block/fdc.c >> @@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv) >> nb_sectors, >> FloppyDriveType_str(parse->drive)); >> } >> + assert(type_match != -1 && "misconfigured fd_format"); >> match = type_match; >> } >> - >> - /* 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)); >> - } >> - >> parse = &(fd_formats[match]); >> >> out: >> > > Truthfully sad to see the lipstick go, because this is my pig. > Oh well, it doesn't matter. Not really, anyway. > > ACK There's still a bit of lipstick in assert()'s argument. v1 has the error_report() lipstick. Would you like me to merge that one instead? Your pig, your choice :)
On 06/08/2018 02:05 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 06/07/2018 10:46 AM, Philippe Mathieu-Daudé wrote: >>> Use assert() 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(). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> hw/block/fdc.c | 9 +-------- >>> 1 file changed, 1 insertion(+), 8 deletions(-) >>> >>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >>> index cd29e27d8f..7c1c57f57f 100644 >>> --- a/hw/block/fdc.c >>> +++ b/hw/block/fdc.c >>> @@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv) >>> nb_sectors, >>> FloppyDriveType_str(parse->drive)); >>> } >>> + assert(type_match != -1 && "misconfigured fd_format"); >>> match = type_match; >>> } >>> - >>> - /* 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)); >>> - } >>> - >>> parse = &(fd_formats[match]); >>> >>> out: >>> >> >> Truthfully sad to see the lipstick go, because this is my pig. >> Oh well, it doesn't matter. Not really, anyway. >> >> ACK > > There's still a bit of lipstick in assert()'s argument. > > v1 has the error_report() lipstick. Would you like me to merge that one > instead? Your pig, your choice :) > In the end I trust your judgement. I like the more verbose error message because it tells you precisely what's going on. I like the assert because it tells you WHERE in the code you've messed up. I am often very keen on making things more usable and less tersely worded. I think you favor some of the same things, but with years more experience than I have. So I defer to you; after all -- they're just pigs :) --js
diff --git a/hw/block/fdc.c b/hw/block/fdc.c index cd29e27d8f..7c1c57f57f 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv) nb_sectors, FloppyDriveType_str(parse->drive)); } + assert(type_match != -1 && "misconfigured fd_format"); match = type_match; } - - /* 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)); - } - parse = &(fd_formats[match]); out:
Use assert() 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(). Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/block/fdc.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)