From 6961bd61cac50117872f6cf43585ed594e45abb1 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 27 Oct 2016 21:59:17 +0200 Subject: [PATCH] LocateUri: migrate from class Error to C++ exceptions --- src/LocateUri.cxx | 60 +++++++++++++------------------- src/LocateUri.hxx | 26 +++----------- src/Partition.hxx | 5 ++- src/PlaylistFile.cxx | 11 ++---- src/PlaylistFile.hxx | 8 +++-- src/SongLoader.cxx | 14 +++----- src/SongLoader.hxx | 5 ++- src/command/CommandError.cxx | 3 -- src/command/FileCommands.cxx | 10 ++---- src/command/OtherCommands.cxx | 20 ++++------- src/command/PlaylistCommands.cxx | 3 +- src/command/QueueCommands.cxx | 15 +++----- src/playlist/PlaylistSong.cxx | 3 +- src/queue/Playlist.hxx | 7 ++-- src/queue/PlaylistEdit.cxx | 8 ++--- test/test_translate_song.cxx | 8 ++--- 16 files changed, 74 insertions(+), 132 deletions(-) diff --git a/src/LocateUri.cxx b/src/LocateUri.cxx index 9a08632c1..43b275671 100644 --- a/src/LocateUri.cxx +++ b/src/LocateUri.cxx @@ -24,25 +24,19 @@ #include "ls.hxx" #include "util/UriUtil.hxx" #include "util/StringCompare.hxx" -#include "util/Error.hxx" -#include "util/Domain.hxx" #ifdef ENABLE_DATABASE #include "storage/StorageInterface.hxx" #endif -const Domain locate_uri_domain("locate_uri"); - static LocatedUri -LocateFileUri(const char *uri, const Client *client, +LocateFileUri(const char *uri, const Client *client #ifdef ENABLE_DATABASE - const Storage *storage, + , const Storage *storage #endif - Error &error) + ) { - auto path = AllocatedPath::FromUTF8(uri, error); - if (path.IsNull()) - return LocatedUri::Unknown(); + auto path = AllocatedPath::FromUTF8Throw(uri); #ifdef ENABLE_DATABASE if (storage != nullptr) { @@ -54,23 +48,21 @@ LocateFileUri(const char *uri, const Client *client, } #endif - if (client != nullptr && !client->AllowFile(path, error)) - return LocatedUri::Unknown(); + if (client != nullptr) + client->AllowFile(path); return LocatedUri(LocatedUri::Type::PATH, uri, std::move(path)); } static LocatedUri -LocateAbsoluteUri(const char *uri, +LocateAbsoluteUri(const char *uri #ifdef ENABLE_DATABASE - const Storage *storage, + , const Storage *storage #endif - Error &error) + ) { - if (!uri_supported_scheme(uri)) { - error.Set(locate_uri_domain, "Unsupported URI scheme"); - return LocatedUri::Unknown(); - } + if (!uri_supported_scheme(uri)) + throw std::runtime_error("Unsupported URI scheme"); #ifdef ENABLE_DATABASE if (storage != nullptr) { @@ -84,37 +76,35 @@ LocateAbsoluteUri(const char *uri, } LocatedUri -LocateUri(const char *uri, const Client *client, +LocateUri(const char *uri, const Client *client #ifdef ENABLE_DATABASE - const Storage *storage, + , const Storage *storage #endif - Error &error) + ) { /* skip the obsolete "file://" prefix */ const char *path_utf8 = StringAfterPrefix(uri, "file://"); if (path_utf8 != nullptr) { - if (!PathTraitsUTF8::IsAbsolute(path_utf8)) { - error.Set(locate_uri_domain, "Malformed file:// URI"); - return LocatedUri::Unknown(); - } + if (!PathTraitsUTF8::IsAbsolute(path_utf8)) + throw std::runtime_error("Malformed file:// URI"); - return LocateFileUri(path_utf8, client, + return LocateFileUri(path_utf8, client #ifdef ENABLE_DATABASE - storage, + , storage #endif - error); + ); } else if (PathTraitsUTF8::IsAbsolute(uri)) - return LocateFileUri(uri, client, + return LocateFileUri(uri, client #ifdef ENABLE_DATABASE - storage, + , storage #endif - error); + ); else if (uri_has_scheme(uri)) - return LocateAbsoluteUri(uri, + return LocateAbsoluteUri(uri #ifdef ENABLE_DATABASE - storage, + , storage #endif - error); + ); else return LocatedUri(LocatedUri::Type::RELATIVE, uri); } diff --git a/src/LocateUri.hxx b/src/LocateUri.hxx index c6aa07443..0d6fed14e 100644 --- a/src/LocateUri.hxx +++ b/src/LocateUri.hxx @@ -35,8 +35,6 @@ #endif #endif -class Domain; -class Error; class Client; #ifdef ENABLE_DATABASE @@ -45,11 +43,6 @@ class Storage; struct LocatedUri { enum class Type { - /** - * Failed to parse the URI. - */ - UNKNOWN, - /** * An absolute URI with a supported scheme. */ @@ -76,22 +69,13 @@ struct LocatedUri { LocatedUri(Type _type, const char *_uri, AllocatedPath &&_path=AllocatedPath::Null()) :type(_type), canonical_uri(_uri), path(std::move(_path)) {} - - gcc_const - static LocatedUri Unknown() { - return LocatedUri(Type::UNKNOWN, nullptr); - } - - bool IsUnknown() const { - return type == Type::UNKNOWN; - } }; -extern const Domain locate_uri_domain; - /** * Classify a URI. * + * Throws #std::runtime_error on error. + * * @param client the #Client that is used to determine whether a local * file is allowed; nullptr disables the check and allows all local * files @@ -100,10 +84,10 @@ extern const Domain locate_uri_domain; * that feature is disabled if this parameter is nullptr */ LocatedUri -LocateUri(const char *uri, const Client *client, +LocateUri(const char *uri, const Client *client #ifdef ENABLE_DATABASE - const Storage *storage, + , const Storage *storage #endif - Error &error); + ); #endif diff --git a/src/Partition.hxx b/src/Partition.hxx index fb21fa69e..9a47f23d1 100644 --- a/src/Partition.hxx +++ b/src/Partition.hxx @@ -68,9 +68,8 @@ struct Partition final : QueueListener, PlayerListener, MixerListener { } unsigned AppendURI(const SongLoader &loader, - const char *uri_utf8, - Error &error) { - return playlist.AppendURI(pc, loader, uri_utf8, error); + const char *uri_utf8) { + return playlist.AppendURI(pc, loader, uri_utf8); } void DeletePosition(unsigned position) { diff --git a/src/PlaylistFile.cxx b/src/PlaylistFile.cxx index 069b1fce8..3c29d1003 100644 --- a/src/PlaylistFile.cxx +++ b/src/PlaylistFile.cxx @@ -373,17 +373,12 @@ try { throw; } -bool +void spl_append_uri(const char *utf8file, - const SongLoader &loader, const char *url, - Error &error) + const SongLoader &loader, const char *url) { - std::unique_ptr song(loader.LoadSong(url, error)); - if (song == nullptr) - return false; - + std::unique_ptr song(loader.LoadSong(url)); spl_append_song(utf8file, *song); - return true; } static void diff --git a/src/PlaylistFile.hxx b/src/PlaylistFile.hxx index d86021b94..6fa846849 100644 --- a/src/PlaylistFile.hxx +++ b/src/PlaylistFile.hxx @@ -73,10 +73,12 @@ spl_remove_index(const char *utf8path, unsigned pos); void spl_append_song(const char *utf8path, const DetachedSong &song); -bool +/** + * Throws #std::runtime_error on error. + */ +void spl_append_uri(const char *path_utf8, - const SongLoader &loader, const char *uri_utf8, - Error &error); + const SongLoader &loader, const char *uri_utf8); void spl_rename(const char *utf8from, const char *utf8to); diff --git a/src/SongLoader.cxx b/src/SongLoader.cxx index a9868f52b..8a5b95920 100644 --- a/src/SongLoader.cxx +++ b/src/SongLoader.cxx @@ -74,9 +74,6 @@ DetachedSong * SongLoader::LoadSong(const LocatedUri &located_uri) const { switch (located_uri.type) { - case LocatedUri::Type::UNKNOWN: - gcc_unreachable(); - case LocatedUri::Type::ABSOLUTE: return new DetachedSong(located_uri.canonical_uri); @@ -91,20 +88,17 @@ SongLoader::LoadSong(const LocatedUri &located_uri) const } DetachedSong * -SongLoader::LoadSong(const char *uri_utf8, Error &error) const +SongLoader::LoadSong(const char *uri_utf8) const { #if !CLANG_CHECK_VERSION(3,6) /* disabled on clang due to -Wtautological-pointer-compare */ assert(uri_utf8 != nullptr); #endif - const auto located_uri = LocateUri(uri_utf8, client, + const auto located_uri = LocateUri(uri_utf8, client #ifdef ENABLE_DATABASE - storage, + , storage #endif - error); - if (located_uri.IsUnknown()) - return nullptr; - + ); return LoadSong(located_uri); } diff --git a/src/SongLoader.hxx b/src/SongLoader.hxx index 392849ce8..409af7b66 100644 --- a/src/SongLoader.hxx +++ b/src/SongLoader.hxx @@ -70,8 +70,11 @@ public: DetachedSong *LoadSong(const LocatedUri &uri) const; + /** + * Throws #std::runtime_error on error. + */ gcc_nonnull_all - DetachedSong *LoadSong(const char *uri_utf8, Error &error) const; + DetachedSong *LoadSong(const char *uri_utf8) const; private: gcc_nonnull_all diff --git a/src/command/CommandError.cxx b/src/command/CommandError.cxx index a22d1f47f..ea1fab039 100644 --- a/src/command/CommandError.cxx +++ b/src/command/CommandError.cxx @@ -21,7 +21,6 @@ #include "CommandError.hxx" #include "PlaylistError.hxx" #include "db/DatabaseError.hxx" -#include "LocateUri.hxx" #include "client/Response.hxx" #include "util/Error.hxx" #include "Log.hxx" @@ -89,8 +88,6 @@ ToAck(const Error &error) { if (error.IsDomain(ack_domain)) { return (enum ack)error.GetCode(); - } else if (error.IsDomain(locate_uri_domain)) { - return ACK_ERROR_ARG; } else if (error.IsDomain(errno_domain)) { return ACK_ERROR_SYSTEM; } diff --git a/src/command/FileCommands.cxx b/src/command/FileCommands.cxx index f9de3b824..f091a3924 100644 --- a/src/command/FileCommands.cxx +++ b/src/command/FileCommands.cxx @@ -216,16 +216,12 @@ handle_read_comments(Client &client, Request args, Response &r) const char *const uri = args.front(); - Error error; - const auto located_uri = LocateUri(uri, &client, + const auto located_uri = LocateUri(uri, &client #ifdef ENABLE_DATABASE - nullptr, + , nullptr #endif - error); + ); switch (located_uri.type) { - case LocatedUri::Type::UNKNOWN: - return print_error(r, error); - case LocatedUri::Type::ABSOLUTE: return read_stream_comments(r, located_uri.canonical_uri); diff --git a/src/command/OtherCommands.cxx b/src/command/OtherCommands.cxx index 47f60601f..a2594fa1c 100644 --- a/src/command/OtherCommands.cxx +++ b/src/command/OtherCommands.cxx @@ -123,17 +123,13 @@ handle_listfiles(Client &client, Request args, Response &r) /* default is root directory */ const auto uri = args.GetOptional(0, ""); - Error error; - const auto located_uri = LocateUri(uri, &client, + const auto located_uri = LocateUri(uri, &client #ifdef ENABLE_DATABASE - nullptr, + , nullptr #endif - error); + ); switch (located_uri.type) { - case LocatedUri::Type::UNKNOWN: - return print_error(r, error); - case LocatedUri::Type::ABSOLUTE: #ifdef ENABLE_DATABASE /* use storage plugin to list remote directory */ @@ -238,17 +234,13 @@ handle_lsinfo(Client &client, Request args, Response &r) compatibility, work around this here */ uri = ""; - Error error; - const auto located_uri = LocateUri(uri, &client, + const auto located_uri = LocateUri(uri, &client #ifdef ENABLE_DATABASE - nullptr, + , nullptr #endif - error); + ); switch (located_uri.type) { - case LocatedUri::Type::UNKNOWN: - return print_error(r, error); - case LocatedUri::Type::ABSOLUTE: return handle_lsinfo_absolute(r, located_uri.canonical_uri); diff --git a/src/command/PlaylistCommands.cxx b/src/command/PlaylistCommands.cxx index 16b3d6e61..1974d7b83 100644 --- a/src/command/PlaylistCommands.cxx +++ b/src/command/PlaylistCommands.cxx @@ -165,7 +165,8 @@ handle_playlistadd(Client &client, Request args, Response &r) Error error; if (uri_has_scheme(uri)) { const SongLoader loader(client); - success = spl_append_uri(playlist, loader, uri, error); + spl_append_uri(playlist, loader, uri); + success = true; } else { #ifdef ENABLE_DATABASE const Database &db = client.GetDatabaseOrThrow(); diff --git a/src/command/QueueCommands.cxx b/src/command/QueueCommands.cxx index 8e51b22d0..f26b34f7c 100644 --- a/src/command/QueueCommands.cxx +++ b/src/command/QueueCommands.cxx @@ -83,16 +83,12 @@ handle_add(Client &client, Request args, Response &r) here */ uri = ""; - Error error; - const auto located_uri = LocateUri(uri, &client, + const auto located_uri = LocateUri(uri, &client #ifdef ENABLE_DATABASE - nullptr, + , nullptr #endif - error); + ); switch (located_uri.type) { - case LocatedUri::Type::UNKNOWN: - return print_error(r, error); - case LocatedUri::Type::ABSOLUTE: case LocatedUri::Type::PATH: AddUri(client, located_uri); @@ -112,10 +108,7 @@ handle_addid(Client &client, Request args, Response &r) const char *const uri = args.front(); const SongLoader loader(client); - Error error; - unsigned added_id = client.partition.AppendURI(loader, uri, error); - if (added_id == 0) - return print_error(r, error); + unsigned added_id = client.partition.AppendURI(loader, uri); if (args.size == 2) { unsigned to = args.ParseUnsigned(1); diff --git a/src/playlist/PlaylistSong.cxx b/src/playlist/PlaylistSong.cxx index a1765125e..08f1dcd22 100644 --- a/src/playlist/PlaylistSong.cxx +++ b/src/playlist/PlaylistSong.cxx @@ -24,7 +24,6 @@ #include "tag/TagBuilder.hxx" #include "fs/Traits.hxx" #include "util/UriUtil.hxx" -#include "util/Error.hxx" #include "DetachedSong.hxx" #include @@ -49,7 +48,7 @@ playlist_check_load_song(DetachedSong &song, const SongLoader &loader) DetachedSong *tmp; try { - tmp = loader.LoadSong(song.GetURI(), IgnoreError()); + tmp = loader.LoadSong(song.GetURI()); } catch (const std::runtime_error &) { return false; } diff --git a/src/queue/Playlist.hxx b/src/queue/Playlist.hxx index 2e9f81cd1..a69f8b52e 100644 --- a/src/queue/Playlist.hxx +++ b/src/queue/Playlist.hxx @@ -209,12 +209,13 @@ public: unsigned AppendSong(PlayerControl &pc, DetachedSong &&song); /** - * @return the new song id or 0 on error + * Throws #std::runtime_error on error. + * + * @return the new song id */ unsigned AppendURI(PlayerControl &pc, const SongLoader &loader, - const char *uri_utf8, - Error &error); + const char *uri_utf8); protected: void DeleteInternal(PlayerControl &pc, diff --git a/src/queue/PlaylistEdit.cxx b/src/queue/PlaylistEdit.cxx index ccfbe85a6..e562fd02e 100644 --- a/src/queue/PlaylistEdit.cxx +++ b/src/queue/PlaylistEdit.cxx @@ -124,13 +124,9 @@ playlist::AppendSong(PlayerControl &pc, DetachedSong &&song) unsigned playlist::AppendURI(PlayerControl &pc, const SongLoader &loader, - const char *uri, - Error &error) + const char *uri) { - std::unique_ptr song(loader.LoadSong(uri, error)); - if (song == nullptr) - return 0; - + std::unique_ptr song(loader.LoadSong(uri)); return AppendSong(pc, std::move(*song)); } diff --git a/test/test_translate_song.cxx b/test/test_translate_song.cxx index 4135cae2c..8b33a5c02 100644 --- a/test/test_translate_song.cxx +++ b/test/test_translate_song.cxx @@ -142,13 +142,13 @@ Client::GetStorage() const return ::storage; } -bool -Client::AllowFile(gcc_unused Path path_fs, gcc_unused Error &error) const +void +Client::AllowFile(gcc_unused Path path_fs) const { - /* always return false, so a SongLoader with a non-nullptr + /* always fail, so a SongLoader with a non-nullptr Client pointer will be regarded "insecure", while one with client==nullptr will allow all files */ - return false; + throw std::runtime_error("foo"); } static std::string