On Thu, Mar 10, 2022 at 03:11:08PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Thu, Mar 10, 2022 at 2:56 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
> > On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> > > On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > > > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> > > > > > The QMP commands have a trailing newline, but the response does
> > not.
> > > > > > This makes the qtest logs hard to follow as the next QMP command
> > > > > > appears in the same line as the previous QMP response.
> > > > > >
> > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > ---
> > > > > > tests/qtest/libqtest.c | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > > > index a85f8a6d05..79c3edcf4b 100644
> > > > > > --- a/tests/qtest/libqtest.c
> > > > > > +++ b/tests/qtest/libqtest.c
> > > > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > > > }
> > > > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > > > }
> > > > > > + if (log) {
> > > > > > + g_assert(write(2, "\n", 1) == 1);
> > > > > > + }
> > > > >
> > > > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> > > >
> > > > You need to check the return value of write() otherwise you'll get a
> > > > compile failure due to a warn_unused_result attribute annotation.
> > > >
> > > > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > > > that in our code.
> > >
> > > You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
> >
> > I don't think we need to do that, per existing common practice:
> >
> > $ git grep '\bg_assert(' | wc -l
> > 2912
> >
> > $ git grep '\bg_assert(' tests | wc -l
> > 2296
> >
> >
> On the topic of assert() usage, it would be nice to state clearly when to
> assert() or g_assert().
>
> g_assert() behaviour is claimed to be more consistent than assert() across
> platforms.
>
> Also -DNDEBUG is less obvious than -DG_DISABLE_CHECKS or -DG_DISABLE_ASSERT.
Personally I would make QEMU build error if NDEBUG or G_DISABLE_ASSERT
was defined, as running with asserts disabled will lead to unsafe code.
We rely on asserts firing to avoid compromise of QEMU when faced with
malicious data.
agreed
> I would remove assert.h and prevent from using it back, but I might be
> missing some reasons to still use it.
As a metric we've got massive use of both families of asset
$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://' | sort | uniq -c
17 assert (
5196 assert(
2913 g_assert(
29 g_assert_cmpfloat(
662 g_assert_cmphex(
1745 g_assert_cmpint(
20 g_assert_cmpmem(
407 g_assert_cmpstr(
756 g_assert_cmpuint(
169 g_assert_false(
138 g_assert_nonnull(
22 g_assert_null(
167 g_assert_true(
But for the tests/ subdir, g_assert family is a clear favourite
$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e 's/.*://' | sort | uniq -c
1 assert (
759 assert(
2297 g_assert(
29 g_assert_cmpfloat(
661 g_assert_cmphex(
1744 g_assert_cmpint(
20 g_assert_cmpmem(
406 g_assert_cmpstr(
754 g_assert_cmpuint(
169 g_assert_false(
138 g_assert_nonnull(
22 g_assert_null(
167 g_assert_true(
This split doesn't appear to have changed all that much over time.
Going back to v3.0.0 we see similar ballpark figures
$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://' | sort | uniq -c
18 assert (
3766 assert(
2210 g_assert(
23 g_assert_cmpfloat(
310 g_assert_cmphex(
1352 g_assert_cmpint(
11 g_assert_cmpmem(
324 g_assert_cmpstr(
489 g_assert_cmpuint(
88 g_assert_false(
73 g_assert_nonnull(
8 g_assert_null(
46 g_assert_true(
[berrange@catbus qemu]$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e 's/.*://' | sort | uniq -c
566 assert(
1876 g_assert(
23 g_assert_cmpfloat(
309 g_assert_cmphex(
1351 g_assert_cmpint(
10 g_assert_cmpmem(
323 g_assert_cmpstr(
488 g_assert_cmpuint(
88 g_assert_false(
73 g_assert_nonnull(
8 g_assert_null(
46 g_assert_true(
Removing either 'assert' or g_assert would be a massive amount of code
churn, for no real functional benefit.
I would personally encourage the more specific g_assert_* variants, to
improve the error messages, but that's really minor.
IMHO we can accept that all of the above are valid to use and worry
about more important things.
Beside the usage inconsistency & the potential platform inconsistencies, it makes code reorganization/split (libslirp-like) a bit more annoying, because <assert.h> is only included in osdep.h.
Also g_assert*() family of functions help more precise usage patterns. g_assert_not_reached() is better than assert(false) or just abort() imho (it seems we use both).