Fix errno race condition and logging macro issues#575
Conversation
|
@mtrmac PTAL |
mtrmac
left a comment
There was a problem hiding this comment.
Locally this looks good, except that strerror is not thread-safe. Is this all guaranteed to run on the main thread?
I didn’t find anything to suggest multi-threaded use, the code seems to be centered around a GMainLoop, but I am not familiar with the codebase.
be021e1 to
822ff00
Compare
|
The conmon is single thread - just updated the implementation so that it's thread safe if we decide to go that way. |
|
LGTM |
src/utils.h
Outdated
| { | ||
| static __thread char errbuf[256]; | ||
| int saved_errno = errno; | ||
| snprintf(errbuf, sizeof(errbuf), "%s", strerror(saved_errno)); |
There was a problem hiding this comment.
if … >= sizeof(errbuf) { abort() } or something? (sprintf("[unexpected error %d"]) is an option but unreasonable)
src/utils.h
Outdated
|
|
||
| static inline const char *get_errno_string(void) | ||
| { | ||
| static __thread char errbuf[256]; |
There was a problem hiding this comment.
(Absolutely unblocking: The __thread works but it seems unnecessary, the caller can allocate a buffer on stack.)
src/utils.h
Outdated
| { | ||
| static __thread char errbuf[256]; | ||
| int saved_errno = errno; | ||
| snprintf(errbuf, sizeof(errbuf), "%s", strerror(saved_errno)); |
There was a problem hiding this comment.
This does not avoid the usage of strerror, so I don’t think this is an improvement compared to the previous version?!
Using %m here might work.
- Fix errno race condition in logging macros - Fix pwarnf macro structure to prevent control flow issues - Add missing log level check to pwarn macro - Fix syslog tag inconsistency in pwarnf - Remove trailing spaces from syslog format strings - Add missing colon in nexit stderr format Fixes: containers#574 Signed-off-by: Jindrich Novy <jnovy@redhat.com>
|
Ok, let's do it in the simple way without any helper functions and exactly what it has to do. |
|
Fine :) I’ll read that carefully later, but this clearly works. |
Fixes: #574