From: Gregor Jasny Date: Mon, 5 Jul 2021 20:08:46 +0000 (+0200) Subject: Use URL class for URL parsing X-Git-Tag: v4.4~152^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F881%2Fhead;p=thirdparty%2Fccache.git Use URL class for URL parsing --- diff --git a/src/storage/Storage.cpp b/src/storage/Storage.cpp index d4d5d953e..a8c402386 100644 --- a/src/storage/Storage.cpp +++ b/src/storage/Storage.cpp @@ -28,7 +28,10 @@ #include #include +#include + #include +#include namespace storage { @@ -179,8 +182,7 @@ namespace { struct ParseStorageEntryResult { - std::string scheme; - std::string url; + Url url; storage::AttributeMap attributes; bool read_only = false; }; @@ -190,16 +192,30 @@ struct ParseStorageEntryResult static ParseStorageEntryResult parse_storage_entry(const nonstd::string_view& entry) { - const auto parts = Util::split_into_views(entry, "|"); + const auto parts = + Util::split_into_views(entry, "|", util::Tokenizer::Mode::include_empty); + + if (parts.empty() || parts.front().empty()) { + throw Error("secondary storage config must provide a URL: {}", entry); + } ParseStorageEntryResult result; - result.url = parts.empty() ? "" : std::string(parts[0]); - const auto colon_slash_slash_pos = result.url.find("://"); - if (colon_slash_slash_pos != std::string::npos) { - result.scheme = result.url.substr(0, colon_slash_slash_pos); + result.url = std::string(parts[0]); + // Url class is parsing the URL object lazily; check if successful + try { + std::ignore = result.url.host(); + } catch (Url::parse_error& e) { + throw Error("Cannot parse URL: {}", e.what()); + } + + if (result.url.scheme().empty()) { + throw Error("URL scheme must not be empty: {}", entry); } for (size_t i = 1; i < parts.size(); ++i) { + if (parts[i].empty()) { + continue; + } const auto kv_pair = util::split_once(parts[i], '='); const auto& key = kv_pair.first; const auto& value = kv_pair.second ? *kv_pair.second : "true"; @@ -220,7 +236,7 @@ parse_storage_entry(const nonstd::string_view& entry) static std::unique_ptr create_storage(const ParseStorageEntryResult& storage_entry) { - if (storage_entry.scheme == "file") { + if (storage_entry.url.scheme() == "file") { return std::make_unique(storage_entry.url, storage_entry.attributes); } @@ -234,11 +250,12 @@ Storage::add_secondary_storages() for (const auto& entry : util::Tokenizer(m_config.secondary_storage(), " ")) { const auto storage_entry = parse_storage_entry(entry); auto storage = create_storage(storage_entry); + auto url_for_logging = storage_entry.url.str(); if (!storage) { - throw Error("unknown secondary storage URL: {}", storage_entry.url); + throw Error("unknown secondary storage URL: {}", url_for_logging); } m_secondary_storages.push_back(SecondaryStorageEntry{ - std::move(storage), storage_entry.url, storage_entry.read_only}); + std::move(storage), url_for_logging, storage_entry.read_only}); } } diff --git a/src/storage/Storage.hpp b/src/storage/Storage.hpp index f15166aad..ca1969312 100644 --- a/src/storage/Storage.hpp +++ b/src/storage/Storage.hpp @@ -59,7 +59,7 @@ private: struct SecondaryStorageEntry { std::unique_ptr backend; - std::string url; + std::string url; // the Url class has a too-chatty stream operator bool read_only = false; }; diff --git a/src/storage/secondary/FileStorage.cpp b/src/storage/secondary/FileStorage.cpp index 677dc3b97..3c0e5985b 100644 --- a/src/storage/secondary/FileStorage.cpp +++ b/src/storage/secondary/FileStorage.cpp @@ -34,14 +34,13 @@ namespace storage { namespace secondary { static std::string -parse_url(const std::string& url) +parse_url(const Url& url) { - const nonstd::string_view prefix = "file://"; - ASSERT(Util::starts_with(url, prefix)); - const auto dir = url.substr(prefix.size()); + ASSERT(url.scheme() == "file"); + const auto& dir = url.path(); if (!Util::starts_with(dir, "/")) { - throw Error("invalid file URL \"{}\" - directory must start with a slash", - url); + throw Error("invalid file path \"{}\" - directory must start with a slash", + dir); } return dir; } @@ -70,7 +69,7 @@ parse_update_mtime(const AttributeMap& attributes) return it != attributes.end() && it->second == "true"; } -FileStorage::FileStorage(const std::string& url, const AttributeMap& attributes) +FileStorage::FileStorage(const Url& url, const AttributeMap& attributes) : m_dir(parse_url(url)), m_umask(parse_umask(attributes)), m_update_mtime(parse_update_mtime(attributes)) diff --git a/src/storage/secondary/FileStorage.hpp b/src/storage/secondary/FileStorage.hpp index 18527b3eb..bcf9de1ed 100644 --- a/src/storage/secondary/FileStorage.hpp +++ b/src/storage/secondary/FileStorage.hpp @@ -21,13 +21,15 @@ #include #include +#include + namespace storage { namespace secondary { class FileStorage : public storage::SecondaryStorage { public: - FileStorage(const std::string& url, const AttributeMap& attributes); + FileStorage(const Url& url, const AttributeMap& attributes); nonstd::expected, Error> get(const Digest& key) override; diff --git a/src/third_party/CMakeLists.txt b/src/third_party/CMakeLists.txt index f8a041e26..3a06ee276 100644 --- a/src/third_party/CMakeLists.txt +++ b/src/third_party/CMakeLists.txt @@ -1,4 +1,4 @@ -add_library(third_party_lib STATIC base32hex.c format.cpp xxhash.c url.cpp) +add_library(third_party_lib STATIC base32hex.c format.cpp url.cpp xxhash.c) if(NOT MSVC) target_sources(third_party_lib PRIVATE getopt_long.c) else() diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 3dd1a36bd..6353319a7 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -66,6 +66,7 @@ addtest(readonly) addtest(readonly_direct) addtest(sanitize_blacklist) addtest(secondary_file) +addtest(secondary_url) addtest(serialize_diagnostics) addtest(source_date_epoch) addtest(split_dwarf) diff --git a/test/suites/secondary_url.bash b/test/suites/secondary_url.bash new file mode 100644 index 000000000..74fe08294 --- /dev/null +++ b/test/suites/secondary_url.bash @@ -0,0 +1,33 @@ +SUITE_secondary_url_SETUP() { + generate_code 1 test.c +} + +SUITE_secondary_url() { + # ------------------------------------------------------------------------- + TEST "Reject empty url (without config attributes)" + + export CCACHE_SECONDARY_STORAGE="|" + $CCACHE_COMPILE -c test.c 2>stderr.log + expect_contains stderr.log "must provide a URL" + + # ------------------------------------------------------------------------- + TEST "Reject empty url (but with config attributes)" + + export CCACHE_SECONDARY_STORAGE="|key=value" + $CCACHE_COMPILE -c test.c 2>stderr.log + expect_contains stderr.log "must provide a URL" + + # ------------------------------------------------------------------------- + TEST "Reject invalid url" + + export CCACHE_SECONDARY_STORAGE="://qwerty" + $CCACHE_COMPILE -c test.c 2>stderr.log + expect_contains stderr.log "Cannot parse URL" + + # ------------------------------------------------------------------------- + TEST "Reject missing scheme" + + export CCACHE_SECONDARY_STORAGE="/qwerty" + $CCACHE_COMPILE -c test.c 2>stderr.log + expect_contains stderr.log "URL scheme must not be empty" +}