| Message ID | 20170818211542.5380-3-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi Eric, On 08/18/2017 06:15 PM, Eric Blake wrote: > Assertions should be separate from the side effects, since in > theory, g_assert() can be disabled (in practice, we can't really > ever do that). What about the suggestion on your "Hacks for building on gcc 7 / Fedora 26" series about avoid building without assertions? The obvious win is a more readable code. http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > qtest.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++------------------- > 1 file changed, 57 insertions(+), 23 deletions(-) > > diff --git a/qtest.c b/qtest.c > index 88a09e9afc..cbbfb71114 100644 > --- a/qtest.c > +++ b/qtest.c > @@ -332,10 +332,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > strcmp(words[0], "outl") == 0) { > unsigned long addr; > unsigned long value; > + int ret; > > g_assert(words[1] && words[2]); > - g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0); > - g_assert(qemu_strtoul(words[2], NULL, 0, &value) == 0); > + ret = qemu_strtoul(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > + ret = qemu_strtoul(words[2], NULL, 0, &value); > + g_assert(ret == 0); > g_assert(addr <= 0xffff); > > if (words[0][3] == 'b') { > @@ -352,9 +355,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > strcmp(words[0], "inl") == 0) { > unsigned long addr; > uint32_t value = -1U; > + int ret; > > g_assert(words[1]); > - g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0); > + ret = qemu_strtoul(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > g_assert(addr <= 0xffff); > > if (words[0][2] == 'b') { > @@ -372,10 +377,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > strcmp(words[0], "writeq") == 0) { > uint64_t addr; > uint64_t value; > + int ret; > > g_assert(words[1] && words[2]); > - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); > - g_assert(qemu_strtou64(words[2], NULL, 0, &value) == 0); > + ret = qemu_strtou64(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > + ret = qemu_strtou64(words[2], NULL, 0, &value); > + g_assert(ret == 0); > > if (words[0][5] == 'b') { > uint8_t data = value; > @@ -401,9 +409,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > strcmp(words[0], "readq") == 0) { > uint64_t addr; > uint64_t value = UINT64_C(-1); > + int ret; > > g_assert(words[1]); > - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); > + ret = qemu_strtou64(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > > if (words[0][4] == 'b') { > uint8_t data; > @@ -427,10 +437,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > uint64_t addr, len, i; > uint8_t *data; > char *enc; > + int ret; > > g_assert(words[1] && words[2]); > - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); > - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); > + ret = qemu_strtou64(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > + ret = qemu_strtou64(words[2], NULL, 0, &len); > + g_assert(ret == 0); > /* We'd send garbage to libqtest if len is 0 */ > g_assert(len); > > @@ -451,10 +464,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > uint64_t addr, len; > uint8_t *data; > gchar *b64_data; > + int ret; > > g_assert(words[1] && words[2]); > - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); > - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); > + ret = qemu_strtou64(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > + ret = qemu_strtou64(words[2], NULL, 0, &len); > + g_assert(ret == 0); > > data = g_malloc(len); > cpu_physical_memory_read(addr, data, len); > @@ -468,10 +484,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > uint64_t addr, len, i; > uint8_t *data; > size_t data_len; > + int ret; > > g_assert(words[1] && words[2] && words[3]); > - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); > - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); > + ret = qemu_strtou64(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > + ret = qemu_strtou64(words[2], NULL, 0, &len); > + g_assert(ret == 0); > > data_len = strlen(words[3]); > if (data_len < 3) { > @@ -497,11 +516,15 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > uint64_t addr, len; > uint8_t *data; > unsigned long pattern; > + int ret; > > g_assert(words[1] && words[2] && words[3]); > - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); > - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); > - g_assert(qemu_strtoul(words[3], NULL, 0, &pattern) == 0); > + ret = qemu_strtou64(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > + ret = qemu_strtou64(words[2], NULL, 0, &len); > + g_assert(ret == 0); > + ret = qemu_strtoul(words[3], NULL, 0, &pattern); > + g_assert(ret == 0); > > if (len) { > data = g_malloc(len); > @@ -517,10 +540,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > uint8_t *data; > size_t data_len; > gsize out_len; > + int ret; > > g_assert(words[1] && words[2] && words[3]); > - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); > - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); > + ret = qemu_strtou64(words[1], NULL, 0, &addr); > + g_assert(ret == 0); > + ret = qemu_strtou64(words[2], NULL, 0, &len); > + g_assert(ret == 0); > > data_len = strlen(words[3]); > if (data_len < 3) { > @@ -551,11 +577,16 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > } else if (strcmp(words[0], "rtas") == 0) { > uint64_t res, args, ret; > unsigned long nargs, nret; > + int rc; > > - g_assert(qemu_strtoul(words[2], NULL, 0, &nargs) == 0); > - g_assert(qemu_strtou64(words[3], NULL, 0, &args) == 0); > - g_assert(qemu_strtoul(words[4], NULL, 0, &nret) == 0); > - g_assert(qemu_strtou64(words[5], NULL, 0, &ret) == 0); > + rc = qemu_strtoul(words[2], NULL, 0, &nargs); > + g_assert(rc == 0); > + rc = qemu_strtou64(words[3], NULL, 0, &args); > + g_assert(rc == 0); > + rc = qemu_strtoul(words[4], NULL, 0, &nret); > + g_assert(rc == 0); > + rc = qemu_strtou64(words[5], NULL, 0, &ret); > + g_assert(rc == 0); > res = qtest_rtas_call(words[1], nargs, args, nret, ret); > > qtest_send_prefix(chr); > @@ -565,7 +596,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > int64_t ns; > > if (words[1]) { > - g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0); > + int ret = qemu_strtoi64(words[1], NULL, 0, &ns); > + g_assert(ret == 0); > } else { > ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL); > } > @@ -575,9 +607,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) { > int64_t ns; > + int ret; > > g_assert(words[1]); > - g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0); > + ret = qemu_strtoi64(words[1], NULL, 0, &ns); > + g_assert(ret == 0); > qtest_clock_warp(ns); > qtest_send_prefix(chr); > qtest_sendf(chr, "OK %"PRIi64"\n", >
On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote: > Hi Eric, > > On 08/18/2017 06:15 PM, Eric Blake wrote: >> Assertions should be separate from the side effects, since in >> theory, g_assert() can be disabled (in practice, we can't really >> ever do that). > > What about the suggestion on your "Hacks for building on gcc 7 / Fedora > 26" series about avoid building without assertions? NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use here) - I have to double-check glib documentation to see whether g_assert() can be crippled in a manner similar to how I know assert() can be crippled. Ideally, we have a form that always performs side effects exactly once, whether or not abort-on-error is enabled (assert() does not have that, but I don't know whether glib does). > > The obvious win is a more readable code. > > http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html Indeed, that's a patch proposal that I still haven't written, but it can be done independently of this series. Still, even if we guarantee that assertions will never be crippled for qemu, it is bad practice to code side-effects in an assert(), because it makes the code harder to copy-and-paste into projects that DO work with assertions disabled, and it raises red flags in reviewers' minds of whether it was intentional. [And truth be told, this particular patch is not really about libqtest, so much as something that horrified me when I was grepping for qemu_strtoul() in order to fix the checkpatch warning I got on libqtest's raw use of strtol() in patch 6, and which necessitated me to write patch 5 - even though the name of the file in question is 'qtest.c']
On 08/18/2017 06:39 PM, Eric Blake wrote: > On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote: >> Hi Eric, >> >> On 08/18/2017 06:15 PM, Eric Blake wrote: >>> Assertions should be separate from the side effects, since in >>> theory, g_assert() can be disabled (in practice, we can't really >>> ever do that). >> >> What about the suggestion on your "Hacks for building on gcc 7 / Fedora >> 26" series about avoid building without assertions? > > NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use > here) - I have to double-check glib documentation to see whether > g_assert() can be crippled in a manner similar to how I know assert() > can be crippled. Ideally, we have a form that always performs side > effects exactly once, whether or not abort-on-error is enabled (assert() > does not have that, but I don't know whether glib does). Yes it does: https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert "The macro can be turned off in final releases of code by defining G_DISABLE_ASSERT when compiling the application, so code must not depend on any side effects from expr ." > >> >> The obvious win is a more readable code. >> >> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html > > Indeed, that's a patch proposal that I still haven't written, but it can > be done independently of this series. > > Still, even if we guarantee that assertions will never be crippled for > qemu, it is bad practice to code side-effects in an assert(), because it > makes the code harder to copy-and-paste into projects that DO work with > assertions disabled, and it raises red flags in reviewers' minds of > whether it was intentional. This is a good point. > [And truth be told, this particular patch is not really about libqtest, > so much as something that horrified me when I was grepping for > qemu_strtoul() in order to fix the checkpatch warning I got on > libqtest's raw use of strtol() in patch 6, and which necessitated me to > write patch 5 - even though the name of the file in question is 'qtest.c'] >
On 08/18/2017 04:39 PM, Eric Blake wrote: > On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote: >> Hi Eric, >> >> On 08/18/2017 06:15 PM, Eric Blake wrote: >>> Assertions should be separate from the side effects, since in >>> theory, g_assert() can be disabled (in practice, we can't really >>> ever do that). >> >> What about the suggestion on your "Hacks for building on gcc 7 / Fedora >> 26" series about avoid building without assertions? > > NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use > here) - I have to double-check glib documentation to see whether > g_assert() can be crippled in a manner similar to how I know assert() > can be crippled. Ideally, we have a form that always performs side > effects exactly once, whether or not abort-on-error is enabled (assert() > does not have that, but I don't know whether glib does). Per https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert, G_DISABLE_ASSERT has the same compile-time effect on g_assert() as NDEBUG on assert(). Even worse, g_assert_not_reached() has the same problem. Then there is the runtime switch g_test_set_nonfatal_assertions() which affects the REST of the g_assert_*() family (such as g_assert_cmpint(), g_assert_true(), g_assert_null()) - but NOT g_assert() proper. And I recall Markus has posted in the past about complaining that g_assert_cmpint() should not be used outside of tests/.
On 08/18/2017 04:58 PM, Eric Blake wrote: >> NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use >> here) - I have to double-check glib documentation to see whether >> g_assert() can be crippled in a manner similar to how I know assert() >> can be crippled. Ideally, we have a form that always performs side >> effects exactly once, whether or not abort-on-error is enabled (assert() >> does not have that, but I don't know whether glib does). > > Per https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert, > G_DISABLE_ASSERT has the same compile-time effect on g_assert() as > NDEBUG on assert(). Even worse, g_assert_not_reached() has the same > problem. Oh, and I failed to mention: g_assert() has the same problem as assert() - when disabled at compile-time, 'expr' is not evaluated. If you want legible code (by including side-effects within your macro that has optional abort-on-error behavior), then you need some OTHER macro that always performs the evaluation. But if you create such a macro, don't use 'assert' in its name. And I still fail to see why anyone would actually want to disable abort-on-error behavior - once an assertion has gone wrong, continuing execution is liable to hit follow-on problems that are much harder to diagnose than it would have been just quitting at the assertion failure.
diff --git a/qtest.c b/qtest.c index 88a09e9afc..cbbfb71114 100644 --- a/qtest.c +++ b/qtest.c @@ -332,10 +332,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) strcmp(words[0], "outl") == 0) { unsigned long addr; unsigned long value; + int ret; g_assert(words[1] && words[2]); - g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0); - g_assert(qemu_strtoul(words[2], NULL, 0, &value) == 0); + ret = qemu_strtoul(words[1], NULL, 0, &addr); + g_assert(ret == 0); + ret = qemu_strtoul(words[2], NULL, 0, &value); + g_assert(ret == 0); g_assert(addr <= 0xffff); if (words[0][3] == 'b') { @@ -352,9 +355,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words) strcmp(words[0], "inl") == 0) { unsigned long addr; uint32_t value = -1U; + int ret; g_assert(words[1]); - g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0); + ret = qemu_strtoul(words[1], NULL, 0, &addr); + g_assert(ret == 0); g_assert(addr <= 0xffff); if (words[0][2] == 'b') { @@ -372,10 +377,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) strcmp(words[0], "writeq") == 0) { uint64_t addr; uint64_t value; + int ret; g_assert(words[1] && words[2]); - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); - g_assert(qemu_strtou64(words[2], NULL, 0, &value) == 0); + ret = qemu_strtou64(words[1], NULL, 0, &addr); + g_assert(ret == 0); + ret = qemu_strtou64(words[2], NULL, 0, &value); + g_assert(ret == 0); if (words[0][5] == 'b') { uint8_t data = value; @@ -401,9 +409,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words) strcmp(words[0], "readq") == 0) { uint64_t addr; uint64_t value = UINT64_C(-1); + int ret; g_assert(words[1]); - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); + ret = qemu_strtou64(words[1], NULL, 0, &addr); + g_assert(ret == 0); if (words[0][4] == 'b') { uint8_t data; @@ -427,10 +437,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) uint64_t addr, len, i; uint8_t *data; char *enc; + int ret; g_assert(words[1] && words[2]); - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); + ret = qemu_strtou64(words[1], NULL, 0, &addr); + g_assert(ret == 0); + ret = qemu_strtou64(words[2], NULL, 0, &len); + g_assert(ret == 0); /* We'd send garbage to libqtest if len is 0 */ g_assert(len); @@ -451,10 +464,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) uint64_t addr, len; uint8_t *data; gchar *b64_data; + int ret; g_assert(words[1] && words[2]); - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); + ret = qemu_strtou64(words[1], NULL, 0, &addr); + g_assert(ret == 0); + ret = qemu_strtou64(words[2], NULL, 0, &len); + g_assert(ret == 0); data = g_malloc(len); cpu_physical_memory_read(addr, data, len); @@ -468,10 +484,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) uint64_t addr, len, i; uint8_t *data; size_t data_len; + int ret; g_assert(words[1] && words[2] && words[3]); - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); + ret = qemu_strtou64(words[1], NULL, 0, &addr); + g_assert(ret == 0); + ret = qemu_strtou64(words[2], NULL, 0, &len); + g_assert(ret == 0); data_len = strlen(words[3]); if (data_len < 3) { @@ -497,11 +516,15 @@ static void qtest_process_command(CharBackend *chr, gchar **words) uint64_t addr, len; uint8_t *data; unsigned long pattern; + int ret; g_assert(words[1] && words[2] && words[3]); - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); - g_assert(qemu_strtoul(words[3], NULL, 0, &pattern) == 0); + ret = qemu_strtou64(words[1], NULL, 0, &addr); + g_assert(ret == 0); + ret = qemu_strtou64(words[2], NULL, 0, &len); + g_assert(ret == 0); + ret = qemu_strtoul(words[3], NULL, 0, &pattern); + g_assert(ret == 0); if (len) { data = g_malloc(len); @@ -517,10 +540,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words) uint8_t *data; size_t data_len; gsize out_len; + int ret; g_assert(words[1] && words[2] && words[3]); - g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0); - g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0); + ret = qemu_strtou64(words[1], NULL, 0, &addr); + g_assert(ret == 0); + ret = qemu_strtou64(words[2], NULL, 0, &len); + g_assert(ret == 0); data_len = strlen(words[3]); if (data_len < 3) { @@ -551,11 +577,16 @@ static void qtest_process_command(CharBackend *chr, gchar **words) } else if (strcmp(words[0], "rtas") == 0) { uint64_t res, args, ret; unsigned long nargs, nret; + int rc; - g_assert(qemu_strtoul(words[2], NULL, 0, &nargs) == 0); - g_assert(qemu_strtou64(words[3], NULL, 0, &args) == 0); - g_assert(qemu_strtoul(words[4], NULL, 0, &nret) == 0); - g_assert(qemu_strtou64(words[5], NULL, 0, &ret) == 0); + rc = qemu_strtoul(words[2], NULL, 0, &nargs); + g_assert(rc == 0); + rc = qemu_strtou64(words[3], NULL, 0, &args); + g_assert(rc == 0); + rc = qemu_strtoul(words[4], NULL, 0, &nret); + g_assert(rc == 0); + rc = qemu_strtou64(words[5], NULL, 0, &ret); + g_assert(rc == 0); res = qtest_rtas_call(words[1], nargs, args, nret, ret); qtest_send_prefix(chr); @@ -565,7 +596,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words) int64_t ns; if (words[1]) { - g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0); + int ret = qemu_strtoi64(words[1], NULL, 0, &ns); + g_assert(ret == 0); } else { ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL); } @@ -575,9 +607,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words) (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) { int64_t ns; + int ret; g_assert(words[1]); - g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0); + ret = qemu_strtoi64(words[1], NULL, 0, &ns); + g_assert(ret == 0); qtest_clock_warp(ns); qtest_send_prefix(chr); qtest_sendf(chr, "OK %"PRIi64"\n",
Assertions should be separate from the side effects, since in theory, g_assert() can be disabled (in practice, we can't really ever do that). Signed-off-by: Eric Blake <eblake@redhat.com> --- qtest.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 57 insertions(+), 23 deletions(-)