Fixes an outrageous number of bugs and warnings #90
Conversation
Signed-off-by: Vinícius Ferrão <[email protected]>
The program is now able to at least boot a compute node again Signed-off-by: Vinícius Ferrão <[email protected]>
include/cloysterhpc/services/log.h
Outdated
| @@ -45,7 +40,12 @@ | |||
| if (spdlog::get(productName) != nullptr) { \ | |||
| spdlog::get(productName)->info(__VA_ARGS__); \ | |||
| } | |||
|
|
|||
| #define LOG_ABORT_IF(cnd, msg) \ | |||
There was a problem hiding this comment.
I don't like the idea of adding logic to a macro.
The idea here is to just print the logs. That's the single responsibility. If you need to do a check, do it on code, print a log an then exits.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-macros2
There was a problem hiding this comment.
Okay I'll remove the macro.
src/os.cpp
Outdated
| if (version.find('.') != std::string::npos) { | ||
| LOG_ABORT("system version (in the answerfile.yml) must follow the format M.N, where M is the major version number and N is the minor version number."); | ||
| } | ||
| LOG_ABORT_IF(version.find('.') == std::string::npos, |
There was a problem hiding this comment.
That's what I'm talking about. Use a if statement and does not hide it behind a macro.
src/repos.cpp
Outdated
| try { | ||
| std::filesystem::copy(dir_entry, "/etc/yum.repos.d"); | ||
| } catch (std::filesystem::filesystem_error const& ex) { | ||
| if (ex.code().message().starts_with("File exists")) { |
There was a problem hiding this comment.
We must not compare string to handle errors. That's very bad design. Text can change, and the language of the glibc may also changes this. We need to aim for portability so a better implementation is to compare the error return code, like this:
if (ex.code().default_error_condition() == std::errc::file_exists) {
continue;
}
// If it's not "file exists", maybe rethrow or handle differently
std::cerr << "Copy failed: " << ex.what() << '\n';
// handle or rethrow
include/cloysterhpc/services/log.h
Outdated
| #define LOG_ABORT(...) \ | ||
| if (spdlog::get(productName) != nullptr) { \ | ||
| spdlog::get(productName)->critical(__VA_ARGS__); \ | ||
| std::exit(-1); \ | ||
| } |
There was a problem hiding this comment.
The macro must not exit. That's not the responsibility of this macro.
* Fix typos in the documentation. * Fix typos on LOG messages. * Change variable names to be more accurate with the context. * Rethrow should also follow the golden rule of: throw by value and catch by reference. Signed-off-by: Vinícius Ferrão <[email protected]>
viniciusferrao
changed the title
src/os.cpp
Outdated
| LOG_CRITICAL("Unexpected value for system.version (in answerfile.ini). " | ||
| "Expected M.N format, e.g., 9.5. Value found instead: {}", | ||
| version); | ||
| std::exit(-1); |
There was a problem hiding this comment.
I don't like this explicit std::exit here. I think this should be an exception and let the exception handler unroll the stack and call std::terminate if it does not find any point of recovery.
It's not the responsibility of this class to call std::exit.
However I didn't changed that myself because I would like to discuss it first.
There was a problem hiding this comment.
Okay, should we have our exception for this or reuse some of the stl?
|
I fixed all the warnings, the tool is running to the end and the node is booting Now we can focus on the code quality issues |
This change will keep the project more reliable while still getting updates from upstream on minor versions. Signed-off-by: Vinícius Ferrão <[email protected]>
This workflow is broken, let's see if an update will fix it. Signed-off-by: Vinícius Ferrão <[email protected]>
Signed-off-by: Vinícius Ferrão <[email protected]>
Signed-off-by: Vinícius Ferrão <[email protected]>
Signed-off-by: Vinícius Ferrão <[email protected]>
|
include/cloysterhpc/services/log.h
Outdated
| @@ -45,7 +40,12 @@ | |||
| if (spdlog::get(productName) != nullptr) { \ | |||
| spdlog::get(productName)->info(__VA_ARGS__); \ | |||
| } | |||
|
|
|||
| #define LOG_ABORT_IF(cnd, msg) \ | |||
There was a problem hiding this comment.
Okay I'll remove the macro.
src/os.cpp
Outdated
| LOG_CRITICAL("Unexpected value for system.version (in answerfile.ini). " | ||
| "Expected M.N format, e.g., 9.5. Value found instead: {}", | ||
| version); | ||
| std::exit(-1); |
There was a problem hiding this comment.
Okay, should we have our exception for this or reuse some of the stl?
|
🚀 |
No description provided.