From e236522de431904894b285ab78ec1ddd5db2d43e Mon Sep 17 00:00:00 2001 From: Timm Fitschen <t.fitschen@indiscale.com> Date: Thu, 22 Jul 2021 14:09:23 +0200 Subject: [PATCH] WIP: fix pedantic errors --- CMakeLists.txt | 66 ++++++++++++++++++- include/caosdb/configuration.h | 2 +- include/caosdb/connection.h | 3 +- include/caosdb/utility.h | 24 +++---- include/ccaosdb.h | 12 ++-- src/caosdb/authentication.cpp | 8 +-- src/caosdb/configuration.cpp | 32 +++++---- src/caosdb/connection.cpp | 6 +- src/ccaosdb.cpp | 12 ++-- test/CMakeLists.txt | 8 ++- ..._client_certificate_file_non_existent.json | 28 ++++++++ test/test_data/test_caosdb_client.json | 2 - 12 files changed, 149 insertions(+), 54 deletions(-) create mode 100644 test/test_data/test_broken_caosdb_client_certificate_file_non_existent.json diff --git a/CMakeLists.txt b/CMakeLists.txt index 91c0423..b41ee6f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -31,7 +31,12 @@ project(libcaosdb DESCRIPTION "C and C++ client libraries for CaosDB" LANGUAGES CXX C) +set(CMAKE_C_STANDARD 99) set(CMAKE_CXX_STANDARD 17) + +set(CMAKE_C_EXTENSIONS OFF) +set(CMAKE_C_STANDARD_REQUIRED ON) +set(CMAKE_CXX_EXTENSIONS OFF) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake" ${CMAKE_MODULE_PATH}) @@ -193,6 +198,39 @@ target_link_libraries(cxxcaosdbcli ### LINTING with CLANG-TIDY and INCLUDE-WHAT-YOU-USE ####################################################### +########################################### +### PARANOID COMPILER SETTINGS +########################################### +option(PARANOID_COMPILER_SETTINGS "Enable extra-paranoid compiler settings +(which may even flag errors for code in the dependencies. These only apply in +Debug BUILD_TYPE with SKIP_LINTING=Off or when LINTING=On." OFF) +include(CheckCXXCompilerFlag) +include(CheckCCompilerFlag) + +function(add_compiler_flag flag) + string(FIND "${CMAKE_CXX_FLAGS}" "${flag}" cxx_present) + if(cxx_present EQUAL -1) + check_cxx_compiler_flag("${flag}" flag_supported) + if(flag_supported) + set(PEDANTIC_CMAKE_CXX_FLAGS "${PEDANTIC_CMAKE_CXX_FLAGS} ${flag}" PARENT_SCOPE) + endif() + unset(flag_supported CACHE) + endif() + unset(cxx_present CACHE) + + string(FIND "${CMAKE_C_FLAGS}" "${flag}" c_present) + if(c_present EQUAL -1) + check_cxx_compiler_flag("${flag}" flag_supported) + if(flag_supported) + set(PEDANTIC_CMAKE_C_FLAGS "${PEDANTIC_CMAKE_C_FLAGS} ${flag}" PARENT_SCOPE) + endif() + unset(flag_supported CACHE) + endif() + unset(c_present CACHE) +endfunction() + + + option(LINTING "Enable linting with clang-tidy and iwyu when in non-Debug build-type" OFF) if("${CMAKE_BUILD_TYPE}" MATCHES "Debug" OR LINTING) set(_LINTING ON) @@ -203,6 +241,27 @@ if("${CMAKE_BUILD_TYPE}" MATCHES "Debug" AND SKIP_LINTING) set(_LINTING OFF) endif() if(_LINTING) + + ### set paranoid compiler flags + add_compiler_flag("-Wall") + add_compiler_flag("-Wextra") + add_compiler_flag("-pedantic") + add_compiler_flag("-Werror") + + set(TARGET_CAOSDB_COMPILE_FLAGS "${TARGET_CAOSDB_COMPILE_FLAGS} ${PEDANTIC_CMAKE_CXX_FLAGS} -isystem /home/tf/.conan/data/grpc/1.38.0/_/_/package/aa66ddfaf24f11a0a60b742fd5533cfa79965e20/include/grpcpp/impl/codegen/server_interface.h") + set(TARGET_CCAOSDB_COMPILE_FLAGS "${TARGET_CCAOSDB_COMPILE_FLAGS} ${PEDANTIC_CMAKE_C_FLAGS}") + set(TARGET_CXXCAOSDBCLI_COMPILE_FLAGS "${TARGET_CXXCAOSDBCLI_COMPILE_FLAGS} ${PEDANTIC_CMAKE_CXX_FLAGS}") + set(TARGET_CCAOSDBCLI_COMPILE_FLAGS "${TARGET_CCAOSDBCLI_COMPILE_FLAGS} ${PEDANTIC_CMAKE_C_FLAGS}") + + set_target_properties(caosdb PROPERTIES + COMPILE_FLAGS "${TARGET_CAOSDB_COMPILE_FLAGS}") + set_target_properties(ccaosdb PROPERTIES + COMPILE_FLAGS "${TARGET_CCAOSDB_COMPILE_FLAGS}") + set_target_properties(cxxcaosdbcli PROPERTIES + COMPILE_FLAGS "${TARGET_CXXCAOSDBCLI_COMPILE_FLAGS}") + set_target_properties(ccaosdbcli PROPERTIES + COMPILE_FLAGS "${TARGET_CCAOSDBCLI_COMPILE_FLAGS}") + find_program(iwyu NAMES include-what-you-use iwyu PATHS ${CMAKE_SOURCE_DIR}/tools/include-what-you-use/${iwyu_os}/bin) @@ -231,9 +290,11 @@ if(_LINTING) message(STATUS "clang-tidy: ${clang_tidy}") set(_CMAKE_CXX_CLANG_TIDY_CHECKS "--checks=*,-fuchsia-*,-llvm-include-order,-llvmlibc-*,-readability-convert-member-functions-to-static,-cppcoreguidelines-pro-bounds-array-to-pointer-decay,-hicpp-no-array-decay,-llvm-else-after-return,-readability-else-after-return") + set(_CMAKE_C_CLANG_TIDY_CHECKS "${_CMAKE_CXX_CLANG_TIDY_CHECKS}") set(_CMAKE_CXX_CLANG_TIDY "${clang_tidy}" "--header-filter=caosdb/.*[^\(\.pb\.h\)]$" "--warnings-as-errors=*") + set(_CMAKE_C_CLANG_TIDY "${_CMAKE_CXX_CLANG_TIDY}") option(AUTO_FIX_LINTING "Append --fix option to clang-tidy" OFF) if(AUTO_FIX_LINTING) set(_CMAKE_CXX_CLANG_TIDY "${_CMAKE_CXX_CLANG_TIDY};--fix") @@ -246,8 +307,11 @@ if(_LINTING) set_target_properties(cxxcaosdbcli PROPERTIES CXX_CLANG_TIDY "${_CMAKE_CXX_CLANG_TIDY};${_CMAKE_CXX_CLANG_TIDY_CHECKS}" ) + set_target_properties(ccaosdb PROPERTIES + C_CLANG_TIDY "${_CMAKE_C_CLANG_TIDY};${_CMAKE_C_CLANG_TIDY_CHECKS}" + ) set_target_properties(ccaosdbcli PROPERTIES - C_CLANG_TIDY "${_CMAKE_CXX_CLANG_TIDY};${_CMAKE_CXX_CLANG_TIDY_CHECKS}" + C_CLANG_TIDY "${_CMAKE_C_CLANG_TIDY};${_CMAKE_C_CLANG_TIDY_CHECKS}" ) endif() endif() diff --git a/include/caosdb/configuration.h b/include/caosdb/configuration.h index 6a59880..8ac50ef 100644 --- a/include/caosdb/configuration.h +++ b/include/caosdb/configuration.h @@ -27,7 +27,7 @@ #include "boost/filesystem/path.hpp" // for path #include "boost/json/object.hpp" // for object #include "boost/json/value.hpp" // for value -#include "boost/json/value_ref.hpp" // for array, object +#include "boost/json/value_ref.hpp" // IWYU pragma: keep #include "caosdb/authentication.h" // for Authenticator, PlainPassw... #include "caosdb/connection.h" // for ConnectionConfiguration, Certifi... #include "caosdb/exceptions.h" // for ConfigurationError diff --git a/include/caosdb/connection.h b/include/caosdb/connection.h index 2c58d57..c81bb72 100644 --- a/include/caosdb/connection.h +++ b/include/caosdb/connection.h @@ -40,6 +40,7 @@ #include "grpcpp/security/credentials.h" // for ChannelCredentials namespace caosdb::connection { +using boost::filesystem::path; using caosdb::authentication::Authenticator; using caosdb::entity::v1alpha1::EntityTransactionService; using caosdb::info::VersionInfo; @@ -58,7 +59,7 @@ private: std::string certificate_provider; public: - explicit PemFileCertificateProvider(const std::string &path); + explicit PemFileCertificateProvider(const path &path); [[nodiscard]] auto GetCertificatePem() const -> std::string override; }; diff --git a/include/caosdb/utility.h b/include/caosdb/utility.h index 5d3ac61..843c1af 100644 --- a/include/caosdb/utility.h +++ b/include/caosdb/utility.h @@ -27,10 +27,12 @@ #include <fstream> #include <string> #include <cstdlib> +#include <memory> #include <boost/json.hpp> #include <boost/beast/core/detail/base64.hpp> #include <boost/filesystem.hpp> #include <boost/filesystem/fstream.hpp> +#include <boost/filesystem/string_file.hpp> namespace caosdb::utility { using boost::filesystem::exists; @@ -44,18 +46,9 @@ using boost::json::value; * * TODO use boost-filesystem's "load_string_file"! */ -inline auto load_string_file(const std::string &path) -> std::string { - const auto path_view = std::string_view{path}; - constexpr auto size = std::size_t{4096}; - auto stream = std::ifstream{path_view.data()}; - stream.exceptions(std::ios_base::badbit); - - auto result = std::string(); - auto buffer = std::string(size, '\0'); - while (stream.read(&buffer[0], size)) { - result.append(buffer, 0, stream.gcount()); - } - result.append(buffer, 0, stream.gcount()); +inline auto load_string_file(const path &path) -> std::string { + std::string result; + boost::filesystem::load_string_file(path, result); return result; } @@ -88,11 +81,12 @@ inline auto base64_encode(const std::string &plain) -> std::string { auto size_plain = plain.size(); auto size_encoded = boost::beast::detail::base64::encoded_size(size_plain); - char encoded[size_encoded]; - boost::beast::detail::base64::encode(&encoded, plain.c_str(), size_plain); + std::unique_ptr<char[]> encoded(new char[size_encoded]); + boost::beast::detail::base64::encode(encoded.get(), plain.c_str(), + size_plain); // the encoded char[] is not null terminated, so explicitely set the length - return std::string(encoded, encoded + size_encoded); + return std::string(encoded.get(), encoded.get() + size_encoded); } inline auto load_json_file(const path &json_file) -> value { diff --git a/include/ccaosdb.h b/include/ccaosdb.h index f9ed3bc..47bf568 100644 --- a/include/ccaosdb.h +++ b/include/ccaosdb.h @@ -7,27 +7,27 @@ extern "C" { /** * Return the constant caosdb::LIBCAOSDB_VERSION_MAJOR. */ -const int caosdb_constants_LIBCAOSDB_VERSION_MAJOR(); +int caosdb_constants_LIBCAOSDB_VERSION_MAJOR(); /** * Return the constant caosdb::LIBCAOSDB_VERSION_MINOR */ -const int caosdb_constants_LIBCAOSDB_VERSION_MINOR(); +int caosdb_constants_LIBCAOSDB_VERSION_MINOR(); /** * Return the constant caosdb::LIBCAOSDB_VERSION_PATCH. */ -const int caosdb_constants_LIBCAOSDB_VERSION_PATCH(); +int caosdb_constants_LIBCAOSDB_VERSION_PATCH(); /** * Return the constant caosdb::COMPATIBLE_SERVER_VERSION_MAJOR. */ -const int caosdb_constants_COMPATIBLE_SERVER_VERSION_MAJOR(); +int caosdb_constants_COMPATIBLE_SERVER_VERSION_MAJOR(); /** * Return the constant caosdb::COMPATIBLE_SERVER_VERSION_MINOR. */ -const int caosdb_constants_COMPATIBLE_SERVER_VERSION_MINOR(); +int caosdb_constants_COMPATIBLE_SERVER_VERSION_MINOR(); /** * Return the constant caosdb::COMPATIBLE_SERVER_VERSION_PATCH. */ -const int caosdb_constants_COMPATIBLE_SERVER_VERSION_PATCH(); +int caosdb_constants_COMPATIBLE_SERVER_VERSION_PATCH(); /** * Return the constant caosdb::COMPATIBLE_SERVER_VERSION_PRE_RELEASE. */ diff --git a/src/caosdb/authentication.cpp b/src/caosdb/authentication.cpp index 7bc70de..db92762 100644 --- a/src/caosdb/authentication.cpp +++ b/src/caosdb/authentication.cpp @@ -36,7 +36,7 @@ using grpc::string_ref; MetadataCredentialsPluginImpl::MetadataCredentialsPluginImpl(std::string key, std::string value) - : key(std::move(key)), value(std::move(value)){}; + : key(std::move(key)), value(std::move(value)) {} auto MetadataCredentialsPluginImpl::GetMetadata( string_ref /*service_url*/, string_ref /*method_name*/, @@ -45,12 +45,12 @@ auto MetadataCredentialsPluginImpl::GetMetadata( metadata->insert(std::make_pair(this->key, this->value)); return Status::OK; -}; +} PlainPasswordAuthenticator::PlainPasswordAuthenticator( const std::string &username, const std::string &password) { this->basic = "Basic " + base64_encode(username + ":" + password); -}; +} auto PlainPasswordAuthenticator::GetCallCredentials() const -> std::shared_ptr<grpc::CallCredentials> { @@ -58,6 +58,6 @@ auto PlainPasswordAuthenticator::GetCallCredentials() const std::unique_ptr<grpc::MetadataCredentialsPlugin>( new MetadataCredentialsPluginImpl("authentication", this->basic))); return call_creds; -}; +} } // namespace caosdb::authentication diff --git a/src/caosdb/configuration.cpp b/src/caosdb/configuration.cpp index fd62e24..3e43b1d 100644 --- a/src/caosdb/configuration.cpp +++ b/src/caosdb/configuration.cpp @@ -53,11 +53,17 @@ auto ConnectionConfigurationHelper::CreateCertificateProvider( if (from.contains("server_certificate_path")) { const value &path_str = from.at("server_certificate_path"); assert(path_str.is_string() == true); + const path certificate_file = path(path_str.as_string().c_str()); + if (!exists(certificate_file)) { + throw ConfigurationError( + "File does not exist (server_certificate_path): " + + certificate_file.string()); + } certificate_provider = std::make_unique<PemFileCertificateProvider>( - std::string(path_str.as_string().c_str())); + path(path_str.as_string().c_str())); } return certificate_provider; -}; +} auto ConnectionConfigurationHelper::CreateAuthenticator( const object &from) const -> std::unique_ptr<Authenticator> { @@ -87,7 +93,7 @@ auto ConnectionConfigurationHelper::CreateAuthenticator( } } return authenticator; -}; +} auto ConnectionConfigurationHelper::CreateConnectionConfiguration( const bool tls, const std::string &host, const int port, @@ -113,7 +119,7 @@ auto ConnectionConfigurationHelper::CreateConnectionConfiguration( } else { return std::make_unique<InsecureConnectionConfiguration>(host, port); } -}; +} auto ConnectionConfigurationHelper::IsTls(const object &from) const -> bool { bool tls = true; @@ -123,7 +129,7 @@ auto ConnectionConfigurationHelper::IsTls(const object &from) const -> bool { tls = tls_switch.as_bool(); } return tls; -}; +} auto ConnectionConfigurationHelper::CreateConnectionConfiguration( const object &from) const -> std::unique_ptr<ConnectionConfiguration> { @@ -145,12 +151,12 @@ auto ConnectionConfigurationHelper::CreateConnectionConfiguration( tls, std::string(host.as_string().c_str()), static_cast<int>(port.as_int64()), certificate_provider.get(), authenticator.get()); -}; +} auto ConfigurationManager::mReset() -> void { mClear(); InitializeDefaults(); -}; +} auto ConfigurationManager::mClear() -> void { json_configuration = value(nullptr); @@ -168,14 +174,14 @@ auto ConfigurationManager::mLoadSingleJSONConfiguration(const path &json_file) json_configuration = load_json_file(json_file); // TODO(far future) validate against json-schema -}; +} auto ConfigurationManager::mGetConnectionConfiguration( const std::string &name) const -> std::unique_ptr<ConnectionConfiguration> { auto connection_json = GetConnection(name); return connection_configuration_helper.CreateConnectionConfiguration( connection_json); -}; +} auto ConfigurationManager::mGetDefaultConnectionName() const -> std::string { auto connections = GetConnections(); @@ -196,7 +202,7 @@ auto ConfigurationManager::mGetDefaultConnectionName() const -> std::string { return connections.begin()->key().to_string(); } throw ConfigurationError("Could not determine the default connection."); -}; +} auto ConfigurationManager::GetConfiguration() const -> const object & { if (json_configuration.is_null()) { @@ -204,7 +210,7 @@ auto ConfigurationManager::GetConfiguration() const -> const object & { } assert(json_configuration.is_object()); return json_configuration.as_object(); -}; +} auto ConfigurationManager::GetConnections() const -> const object & { const auto &configuration = GetConfiguration(); @@ -224,7 +230,7 @@ auto ConfigurationManager::GetConnections() const -> const object & { "This CaosDB client hasn't any configured connections."); } return connections_object; -}; +} auto ConfigurationManager::GetConnection(const std::string &name) const -> const object & { @@ -236,7 +242,7 @@ auto ConfigurationManager::GetConnection(const std::string &name) const } throw ConfigurationError("The connection '" + name + "' has not been defined."); -}; +} auto ConfigurationManager::InitializeDefaults() -> void { diff --git a/src/caosdb/connection.cpp b/src/caosdb/connection.cpp index d2a2ae1..8607857 100644 --- a/src/caosdb/connection.cpp +++ b/src/caosdb/connection.cpp @@ -39,6 +39,7 @@ #include "grpcpp/impl/codegen/status_code_enum.h" // for StatusCode, UNAUTH... namespace caosdb::connection { +using boost::filesystem::path; using caosdb::authentication::Authenticator; using caosdb::configuration::ConfigurationManager; using caosdb::entity::v1alpha1::EntityTransactionService; @@ -54,8 +55,7 @@ using grpc::InsecureChannelCredentials; using grpc::SslCredentials; using grpc::SslCredentialsOptions; -PemFileCertificateProvider::PemFileCertificateProvider( - const std::string &path) { +PemFileCertificateProvider::PemFileCertificateProvider(const path &path) { this->certificate_provider = load_string_file(path); } @@ -197,7 +197,7 @@ auto operator<<(std::ostream &out, const Connection & /*connection*/) -> std::unique_ptr<Transaction> { auto service_stub = this->entity_transaction_service; return std::make_unique<Transaction>(service_stub); -}; +} auto ConnectionManager::mHasConnection(const std::string &name) const -> bool { auto it = connections.find(name); diff --git a/src/ccaosdb.cpp b/src/ccaosdb.cpp index 2c9539a..4888766 100644 --- a/src/ccaosdb.cpp +++ b/src/ccaosdb.cpp @@ -9,27 +9,27 @@ extern "C" { -const int caosdb_constants_LIBCAOSDB_VERSION_MAJOR() { +int caosdb_constants_LIBCAOSDB_VERSION_MAJOR() { return caosdb::LIBCAOSDB_VERSION_MAJOR; } -const int caosdb_constants_LIBCAOSDB_VERSION_MINOR() { +int caosdb_constants_LIBCAOSDB_VERSION_MINOR() { return caosdb::LIBCAOSDB_VERSION_MINOR; } -const int caosdb_constants_LIBCAOSDB_VERSION_PATCH() { +int caosdb_constants_LIBCAOSDB_VERSION_PATCH() { return caosdb::LIBCAOSDB_VERSION_PATCH; } -const int caosdb_constants_COMPATIBLE_SERVER_VERSION_MAJOR() { +int caosdb_constants_COMPATIBLE_SERVER_VERSION_MAJOR() { return caosdb::COMPATIBLE_SERVER_VERSION_MAJOR; } -const int caosdb_constants_COMPATIBLE_SERVER_VERSION_MINOR() { +int caosdb_constants_COMPATIBLE_SERVER_VERSION_MINOR() { return caosdb::COMPATIBLE_SERVER_VERSION_MINOR; } -const int caosdb_constants_COMPATIBLE_SERVER_VERSION_PATCH() { +int caosdb_constants_COMPATIBLE_SERVER_VERSION_PATCH() { return caosdb::COMPATIBLE_SERVER_VERSION_PATCH; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 17613e4..aab1f0a 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -91,8 +91,12 @@ if (LCOV_PATH) GENHTML_ARGS --rc lcov_branch_coverage=1 ) message(STATUS "Adding COMPILE_FLAGS for coverage: ${COVERAGE_COMPILER_FLAGS}") - set_target_properties(caosdb ccaosdb PROPERTIES - COMPILE_FLAGS "${CMAKE_CXX_FLAGS} ${COVERAGE_COMPILER_FLAGS}") + set(TARGET_CAOSDB_COMPILE_FLAGS "${TARGET_CAOSDB_COMPILE_FLAGS} ${COVERAGE_COMPILER_FLAGS}" PARENT_SCOPE) + set(TARGET_CCAOSDB_COMPILE_FLAGS "${TARGET_CCAOSDB_COMPILE_FLAGS} ${COVERAGE_COMPILER_FLAGS}" PARENT_SCOPE) + set_target_properties(caosdb PROPERTIES + COMPILE_FLAGS "${TARGET_CAOSDB_COMPILE_FLAGS}") + set_target_properties(caosdb PROPERTIES + COMPILE_FLAGS "${TARGET_CCAOSDB_COMPILE_FLAGS}") else () message(WARNING "Could not generate code coverage report. Please install lcov.") endif () diff --git a/test/test_data/test_broken_caosdb_client_certificate_file_non_existent.json b/test/test_data/test_broken_caosdb_client_certificate_file_non_existent.json new file mode 100644 index 0000000..832b0a6 --- /dev/null +++ b/test/test_data/test_broken_caosdb_client_certificate_file_non_existent.json @@ -0,0 +1,28 @@ +{ + "connections": { + "default": "local-caosdb", + "local-caosdb-admin": { + "host": "localhost", + "port": 8443, + "server_certificate_path": "some/path/cacert.pem", + "authentication": { + "type": "plain", + "username": "admin", + "password": "caosdb" + } + }, + "local-caosdb": { + "host": "localhost", + "port": 8443, + "server_certificate_path": "some/path/cacert.pem", + "authentication": { + "type": "plain", + "username": "me", + "password": "secret!" + } + } + }, + "extension": { + "this is my": "special option" + } +} diff --git a/test/test_data/test_caosdb_client.json b/test/test_data/test_caosdb_client.json index 832b0a6..10102bc 100644 --- a/test/test_data/test_caosdb_client.json +++ b/test/test_data/test_caosdb_client.json @@ -4,7 +4,6 @@ "local-caosdb-admin": { "host": "localhost", "port": 8443, - "server_certificate_path": "some/path/cacert.pem", "authentication": { "type": "plain", "username": "admin", @@ -14,7 +13,6 @@ "local-caosdb": { "host": "localhost", "port": 8443, - "server_certificate_path": "some/path/cacert.pem", "authentication": { "type": "plain", "username": "me", -- GitLab