basic/log: assert that 0 is not passed as errno, except in test code

Let's assert if we ever happen to pass 0 to one of the log functions.
With the preceding commit to return -EIO from log_*(), passing 0 wouldn't
affect the return value any more, but it is still most likely an error.
The unit test code is an exception: we fairly often pass the return value
to print it, before checking what it is. So let's assert that we're not
passing 0 in non-test code. As with the previous check for %m, this is only
done in developer mode. We are depending on external code setting
errno correctly for us, which might not always be true, and which we can't
test, so we shouldn't assert, but just handle this gracefully.

I did a bunch of greps to try to figure out if there are any places where
we're passing 0 on purpose, and couldn't find any.
The one place that failed in tests is adjusted.

About "zerook" in the name: I wanted the suffix to be unambiguous. It's a
single "word" because each of the words in log_full_errno is also meaningful,
and having one term use two words would be confusing.
This commit is contained in:
Zbigniew Jędrzejewski-Szmek 2021-04-14 15:55:16 +02:00
parent 63275a7032
commit a626cb15c0
4 changed files with 32 additions and 14 deletions

View file

@ -3318,11 +3318,15 @@ custom_target(
'} >@OUTPUT@'], '} >@OUTPUT@'],
build_by_default : true) build_by_default : true)
# We intentionally do not do inline initializations with definitions for test_cflags = ['-DTEST_CODE=1']
# a bunch of _cleanup_ variables in tests, to ensure valgrind is triggered. # We intentionally do not do inline initializations with definitions for a
# This triggers a lot of maybe-uninitialized false positives when the # bunch of _cleanup_ variables in tests, to ensure valgrind is triggered if we
# combination of -O2 and -flto is used. Suppress them. # use the variable unexpectedly. This triggers a lot of maybe-uninitialized
no_uninit = '-O2' in get_option('c_args') and '-flto=auto' in get_option('c_args') ? cc.first_supported_argument('-Wno-maybe-uninitialized') : [] # false positives when the combination of -O2 and -flto is used. Suppress them.
if '-O2' in get_option('c_args') and '-flto=auto' in get_option('c_args')
test_cflags += cc.first_supported_argument('-Wno-maybe-uninitialized')
endif
foreach tuple : tests foreach tuple : tests
sources = tuple[0] sources = tuple[0]
link_with = tuple.length() > 1 and tuple[1].length() > 0 ? tuple[1] : [libshared] link_with = tuple.length() > 1 and tuple[1].length() > 0 ? tuple[1] : [libshared]
@ -3331,7 +3335,7 @@ foreach tuple : tests
condition = tuple.length() > 4 ? tuple[4] : '' condition = tuple.length() > 4 ? tuple[4] : ''
type = tuple.length() > 5 ? tuple[5] : '' type = tuple.length() > 5 ? tuple[5] : ''
defs = tuple.length() > 6 ? tuple[6] : [] defs = tuple.length() > 6 ? tuple[6] : []
defs += no_uninit defs += test_cflags
parallel = tuple.length() > 7 ? tuple[7] : true parallel = tuple.length() > 7 ? tuple[7] : true
timeout = 30 timeout = 30
@ -3399,7 +3403,7 @@ exe = executable(
'test-libudev-sym', 'test-libudev-sym',
test_libudev_sym_c, test_libudev_sym_c,
include_directories : libudev_includes, include_directories : libudev_includes,
c_args : ['-Wno-deprecated-declarations'] + no_uninit, c_args : ['-Wno-deprecated-declarations'] + test_cflags,
link_with : [libudev], link_with : [libudev],
build_by_default : want_tests != 'false', build_by_default : want_tests != 'false',
install : install_tests, install : install_tests,
@ -3412,7 +3416,7 @@ exe = executable(
'test-libudev-static-sym', 'test-libudev-static-sym',
test_libudev_sym_c, test_libudev_sym_c,
include_directories : libudev_includes, include_directories : libudev_includes,
c_args : ['-Wno-deprecated-declarations'] + no_uninit, c_args : ['-Wno-deprecated-declarations'] + test_cflags,
link_with : [install_libudev_static], link_with : [install_libudev_static],
build_by_default : want_tests != 'false' and static_libudev_pic, build_by_default : want_tests != 'false' and static_libudev_pic,
install : install_tests and static_libudev_pic, install : install_tests and static_libudev_pic,
@ -3453,7 +3457,7 @@ foreach tuple : fuzzers
include_directories : [incs, include_directories('src/fuzz')], include_directories : [incs, include_directories('src/fuzz')],
link_with : link_with, link_with : link_with,
dependencies : dependencies, dependencies : dependencies,
c_args : defs, c_args : defs + test_cflags,
link_args: link_args, link_args: link_args,
install : false, install : false,
build_by_default : fuzz_tests or fuzzer_build) build_by_default : fuzz_tests or fuzzer_build)

View file

@ -189,7 +189,7 @@ void log_assert_failed_return(
log_dispatch_internal(level, error, PROJECT_FILE, __LINE__, __func__, NULL, NULL, NULL, NULL, buffer) log_dispatch_internal(level, error, PROJECT_FILE, __LINE__, __func__, NULL, NULL, NULL, NULL, buffer)
/* Logging with level */ /* Logging with level */
#define log_full_errno(level, error, ...) \ #define log_full_errno_zerook(level, error, ...) \
({ \ ({ \
int _level = (level), _e = (error); \ int _level = (level), _e = (error); \
_e = (log_get_max_level() >= LOG_PRI(_level)) \ _e = (log_get_max_level() >= LOG_PRI(_level)) \
@ -198,17 +198,30 @@ void log_assert_failed_return(
_e < 0 ? _e : -EIO; \ _e < 0 ? _e : -EIO; \
}) })
#if BUILD_MODE_DEVELOPER && !defined(TEST_CODE)
# define ASSERT_NON_ZERO(x) assert((x) != 0)
#else
# define ASSERT_NON_ZERO(x)
#endif
#define log_full_errno(level, error, ...) \
({ \
int _error = (error); \
ASSERT_NON_ZERO(_error); \
log_full_errno_zerook(level, _error, __VA_ARGS__); \
})
#define log_full(level, fmt, ...) \ #define log_full(level, fmt, ...) \
({ \ ({ \
if (BUILD_MODE_DEVELOPER) \ if (BUILD_MODE_DEVELOPER) \
assert(!strstr(fmt, "%m")); \ assert(!strstr(fmt, "%m")); \
(void) log_full_errno((level), 0, fmt, ##__VA_ARGS__); \ (void) log_full_errno_zerook(level, 0, fmt, ##__VA_ARGS__); \
}) })
int log_emergency_level(void); int log_emergency_level(void);
/* Normal logging */ /* Normal logging */
#define log_debug(...) log_full_errno(LOG_DEBUG, 0, __VA_ARGS__) #define log_debug(...) log_full_errno_zerook(LOG_DEBUG, 0, __VA_ARGS__)
#define log_info(...) log_full(LOG_INFO, __VA_ARGS__) #define log_info(...) log_full(LOG_INFO, __VA_ARGS__)
#define log_notice(...) log_full(LOG_NOTICE, __VA_ARGS__) #define log_notice(...) log_full(LOG_NOTICE, __VA_ARGS__)
#define log_warning(...) log_full(LOG_WARNING, __VA_ARGS__) #define log_warning(...) log_full(LOG_WARNING, __VA_ARGS__)

View file

@ -306,7 +306,8 @@ int bus_wait_for_jobs(BusWaitForJobs *d, bool quiet, const char* const* extra_ar
if (q < 0 && r == 0) if (q < 0 && r == 0)
r = q; r = q;
log_debug_errno(q, "Got result %s/%m for job %s", d->result, d->name); log_full_errno_zerook(LOG_DEBUG, q,
"Got result %s/%m for job %s", d->result, d->name);
} }
d->name = mfree(d->name); d->name = mfree(d->name);

View file

@ -293,7 +293,7 @@ int main(int argc, char *argv[]) {
r = cg_all_unified(); r = cg_all_unified();
if (r <= 0) if (r <= 0)
return log_tests_skipped_errno(r, "Unified hierarchy is required, skipping."); return log_tests_skipped("Unified hierarchy is required, skipping.");
r = enter_cgroup_subroot(NULL); r = enter_cgroup_subroot(NULL);
if (r == -ENOMEDIUM) if (r == -ENOMEDIUM)