Matt recently raised the issue of ceph assertions in production code, which reminded me of Sage's 2016 pr https://github.com/ceph/ceph/pull/9969 that added a ceph_assert_always(). the idea was to eventually make ceph_assert() conditional on NDEBUG to match the behavior of libc's assert(), leaving ceph_assert_always() as the marked unconditional case i would love to see this finally happen, but there are some potential risks: * ceph_assert()s with side effects won't behave as expected in release builds. assert() documents this same issue at https://www.man7.org/linux/man-pages/man3/assert.3.html#BUGS. if we could at least identify these cases, we can switch them to ceph_assert_always() * in teuthology, we test the same release builds that we ship to users. that means teuthology won't catch the code paths that trigger debug assertions. if those lead to crashes, they could be much harder to debug without the assertions and backtraces * conversely, merging pull requests after a successful teuthology run may introduce new assertions in debug builds. it could be annoying for developers to track down and fix new assertions after pulling the latest main or stable release branch * unused variable warnings in release builds where ceph_assert() was the only reference. at least the compiler will catch all of these for us, and [[maybe_unused]] annotations can clear them up in general, do folks agree that this is a change worth making? if so, what can we do to mitigate the risks? if not, how should we handle the use of ceph_assert() vs raw assert() in new code? should there be some guidance in CodingStyle? as a half-measure, we might introduce a new ceph_assert_debug() as an alternative to raw assert(), then convert some existing uses of ceph_assert() on a case-by-case basis _______________________________________________ Dev mailing list -- dev@xxxxxxx To unsubscribe send an email to dev-leave@xxxxxxx
