- fixed logging during shutdown#415
- fixed logging during shutdown#415aschnell merged 7 commits intoopenSUSE:masterfrom aschnell:master
Conversation
|
+1 but the git commit message subject lines with leading '-' look unusual |
lslezak
left a comment
lslezak
left a comment
There was a problem hiding this comment.
Just some minor questions/comments...
| boost::lock_guard<boost::mutex> lock(logger_data->mutex); | ||
|
|
||
| FILE* f = fopen(filename->c_str(), "ae"); | ||
| FILE* f = fopen(logger_data->filename.c_str(), "ae"); |
There was a problem hiding this comment.
Question, I'm not a C++ expert, there is no C++ equivalent for this plain C fopen?
There was a problem hiding this comment.
There is ofsteam. But "e" flag (O_CLOEXEC) cannot be provided there AFAIR.
There was a problem hiding this comment.
OK, it seems C++ does not support O_CLOEXEC... 😟
server/MetaSnapper.cc
Outdated
| void | ||
| MetaSnappers::unload() | ||
| { | ||
| for (iterator it = entries.begin(); it != entries.end(); ++it) |
There was a problem hiding this comment.
NP: I'd prefer the new syntax without iterators and increment:
for(auto& entry : entries)
entry.unload();There was a problem hiding this comment.
Snapper still uses the old syntax since it was created mainly before C++11. Many more things could maybe be moderized, e.g. use of C++11 regex and thread. That would even remove the dependency on boost-thread.
There was a problem hiding this comment.
OK, but could we use the new syntax while we are touching the code anyway? In YaST we also still have a lot of YCP-like Ruby code but we are gradually rewriting it to pure Ruby when touching it. Would this approach work also for snapper?
There was a problem hiding this comment.
OK, I have changed it to the new C++11 syntax. But given the current activity of the snapper project there will be a mixture of syntaxes for many years.
snapper/Logger.cc
Outdated
|
|
||
| struct LoggerData | ||
| { | ||
| LoggerData() : filename("/var/log/snapper.log"), mutex() {} |
There was a problem hiding this comment.
NP: This string is hardcoded in several places, it would be nice to have a constant for it to have a single place to change. BTW the log file is not configurable?
There was a problem hiding this comment.
Added a define.
No, the log filename is not configurable. You would also need to modify logrotate in that case so it is not easy.
3 participants
For https://trello.com/c/USeFyjd0/100-sle15-p2-1079385-snapperd-dumped-core-with and others.