| Message ID | 20180607144645.10187-3-f4bug@amsat.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | qapi/error: converts error_setg(&error_fatal) to error_report() + exit() | expand |
On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote: > Use 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(). > > Use abort() instead of the suggested assert() because the assertion is > already verified by the switch case. I think g_assert_not_reached() would be the right thing here. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/ppc/spapr_drc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 8a045d6b93..b934b9c9ed 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name, > break; > } > default: > - error_setg(&error_abort, "device FDT in unexpected state: %d", tag); > + abort(); /* device FDT in unexpected state */ > } > fdt_offset = fdt_offset_next; > } while (fdt_depth != 0);
Hi David, On 06/08/2018 12:03 AM, David Gibson wrote: > On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote: >> Use 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(). >> >> Use abort() instead of the suggested assert() because the assertion is >> already verified by the switch case. > > I think g_assert_not_reached() would be the right thing here. I try to follow Eric advice (who recalled Markus). As I understand: http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03605.html "glib-Testing [...] should not be used outside of tests/." I can respin if you prefer. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/ppc/spapr_drc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c >> index 8a045d6b93..b934b9c9ed 100644 >> --- a/hw/ppc/spapr_drc.c >> +++ b/hw/ppc/spapr_drc.c >> @@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name, >> break; >> } >> default: >> - error_setg(&error_abort, "device FDT in unexpected state: %d", tag); >> + abort(); /* device FDT in unexpected state */ >> } >> fdt_offset = fdt_offset_next; >> } while (fdt_depth != 0); >
On Fri, Jun 08, 2018 at 12:54:36AM -0300, Philippe Mathieu-Daudé wrote: > Hi David, > > On 06/08/2018 12:03 AM, David Gibson wrote: > > On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote: > >> Use 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(). > >> > >> Use abort() instead of the suggested assert() because the assertion is > >> already verified by the switch case. > > > > I think g_assert_not_reached() would be the right thing here. > > I try to follow Eric advice (who recalled Markus). > As I understand: > http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03605.html > > "glib-Testing [...] should not be used outside of tests/." Oh, ok, go with that then. Acked-by: David Gibson <david@gibson.dropbear.id.au> > I can respin if you prefer. > > >> > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> --- > >> hw/ppc/spapr_drc.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > >> index 8a045d6b93..b934b9c9ed 100644 > >> --- a/hw/ppc/spapr_drc.c > >> +++ b/hw/ppc/spapr_drc.c > >> @@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name, > >> break; > >> } > >> default: > >> - error_setg(&error_abort, "device FDT in unexpected state: %d", tag); > >> + abort(); /* device FDT in unexpected state */ > >> } > >> fdt_offset = fdt_offset_next; > >> } while (fdt_depth != 0); > > >
David Gibson <david@gibson.dropbear.id.au> writes: > On Fri, Jun 08, 2018 at 12:54:36AM -0300, Philippe Mathieu-Daudé wrote: >> Hi David, >> >> On 06/08/2018 12:03 AM, David Gibson wrote: >> > On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote: >> >> Use 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(). >> >> >> >> Use abort() instead of the suggested assert() because the assertion is >> >> already verified by the switch case. >> > >> > I think g_assert_not_reached() would be the right thing here. >> >> I try to follow Eric advice (who recalled Markus). >> As I understand: >> http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03605.html >> >> "glib-Testing [...] should not be used outside of tests/." > > Oh, ok, go with that then. Most g_assert_FOO() are indeed tests-only, but g_assert_not_reached() is actually acceptable elsewhere, see commit 6e9389563e5, and its review thread https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05499.html Message-Id: <20170427165526.19836-1-dgilbert@redhat.com> That said, I fail to see the value of this kind of lipstick, too. > Acked-by: David Gibson <david@gibson.dropbear.id.au> > >> I can respin if you prefer. I can replace by g_assert_not_reached() on commit if you prefer.
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 8a045d6b93..b934b9c9ed 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name, break; } default: - error_setg(&error_abort, "device FDT in unexpected state: %d", tag); + abort(); /* device FDT in unexpected state */ } fdt_offset = fdt_offset_next; } while (fdt_depth != 0);
Use 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(). Use abort() instead of the suggested assert() because the assertion is already verified by the switch case. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/ppc/spapr_drc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)