From 7bb5af486536e65b33113829d2aadb567e3506d7 Mon Sep 17 00:00:00 2001 From: Thomas Amland Date: Wed, 4 Feb 2015 14:10:15 +0100 Subject: [PATCH 1/2] remove unused method --- xbmc/addons/Repository.cpp | 14 -------------- xbmc/addons/Repository.h | 2 -- 2 files changed, 16 deletions(-) diff --git a/xbmc/addons/Repository.cpp b/xbmc/addons/Repository.cpp index f97aab3110df8..b5d0665f27f32 100644 --- a/xbmc/addons/Repository.cpp +++ b/xbmc/addons/Repository.cpp @@ -104,20 +104,6 @@ CRepository::~CRepository() { } -string CRepository::Checksum() const -{ - /* This code is duplicated in CRepositoryUpdateJob::GrabAddons(). - * If you make changes here, they may be applicable there, too. - */ - string result; - for (DirList::const_iterator it = m_dirs.begin(); it != m_dirs.end(); ++it) - { - if (!it->checksum.empty()) - result += FetchChecksum(it->checksum); - } - return result; -} - string CRepository::FetchChecksum(const string& url) { CFile file; diff --git a/xbmc/addons/Repository.h b/xbmc/addons/Repository.h index 7829d5117b5b8..7331713b77f4d 100644 --- a/xbmc/addons/Repository.h +++ b/xbmc/addons/Repository.h @@ -34,8 +34,6 @@ namespace ADDON CRepository(const cp_extension_t *props); virtual ~CRepository(); - std::string Checksum() const; - /*! \brief Get the md5 hash for an addon. \param the addon in question. \return the md5 hash for the given addon, empty if non exists. From d355b031e02c36127e62d0451e41f57f5134397f Mon Sep 17 00:00:00 2001 From: Thomas Amland Date: Wed, 4 Feb 2015 15:25:39 +0100 Subject: [PATCH 2/2] [repos] handle errors in repo xml parsing and checksum/addons.xml fetching --- xbmc/addons/Repository.cpp | 98 ++++++++++++++++++++------------------ xbmc/addons/Repository.h | 4 +- 2 files changed, 53 insertions(+), 49 deletions(-) diff --git a/xbmc/addons/Repository.cpp b/xbmc/addons/Repository.cpp index b5d0665f27f32..0d0b75a560cf0 100644 --- a/xbmc/addons/Repository.cpp +++ b/xbmc/addons/Repository.cpp @@ -151,11 +151,8 @@ string CRepository::GetAddonHash(const AddonPtr& addon) const x = y; \ } -VECADDONS CRepository::Parse(const DirInfo& dir) +bool CRepository::Parse(const DirInfo& dir, VECADDONS &result) { - VECADDONS result; - CXBMCTinyXML doc; - string file = dir.info; if (dir.compressed) { @@ -167,9 +164,10 @@ VECADDONS CRepository::Parse(const DirInfo& dir) file = url.Get(); } - if (doc.LoadFile(file) && doc.RootElement()) + CXBMCTinyXML doc; + if (doc.LoadFile(file) && doc.RootElement() && + CAddonMgr::Get().AddonsFromRepoXML(doc.RootElement(), result)) { - CAddonMgr::Get().AddonsFromRepoXML(doc.RootElement(), result); for (IVECADDONS i = result.begin(); i != result.end(); ++i) { AddonPtr addon = *i; @@ -190,9 +188,9 @@ VECADDONS CRepository::Parse(const DirInfo& dir) SET_IF_NOT_EMPTY(addon->Props().fanart,URIUtils::AddFileToFolder(dir.datadir,addon->ID()+"/fanart.jpg")) } } + return true; } - - return result; + return false; } void CRepository::OnPostInstall(bool restart, bool update) @@ -237,12 +235,13 @@ bool CRepositoryUpdateJob::DoWork() { if (ShouldCancel(0, 0)) return false; - RepositoryPtr repo = boost::dynamic_pointer_cast(*i); - VECADDONS newAddons = GrabAddons(repo); - MergeAddons(addons, newAddons); + const RepositoryPtr repo = boost::dynamic_pointer_cast(*i); + VECADDONS newAddons; + if (GrabAddons(repo, newAddons)) + MergeAddons(addons, newAddons); } if (addons.empty()) - return false; + return true; //Nothing to do // check for updates CAddonDatabase database; @@ -329,63 +328,68 @@ bool CRepositoryUpdateJob::DoWork() return true; } -VECADDONS CRepositoryUpdateJob::GrabAddons(RepositoryPtr& repo) +bool CRepositoryUpdateJob::GrabAddons(const RepositoryPtr& repo, VECADDONS& addons) { CAddonDatabase database; - VECADDONS addons; database.Open(); - string checksum; - database.GetRepoChecksum(repo->ID(),checksum); - string reposum; + string oldReposum; + if (!database.GetRepoChecksum(repo->ID(), oldReposum)) + oldReposum = ""; - /* This for loop is duplicated in CRepository::Checksum(). - * If you make changes here, they may be applicable there, too. - */ + string reposum; for (CRepository::DirList::const_iterator it = repo->m_dirs.begin(); it != repo->m_dirs.end(); ++it) { if (ShouldCancel(0, 0)) - return addons; + return false; if (!it->checksum.empty()) - reposum += CRepository::FetchChecksum(it->checksum); + { + const string dirsum = CRepository::FetchChecksum(it->checksum); + if (dirsum.empty()) + { + CLog::Log(LOGERROR, "Failed to fetch checksum for directory listing %s for repository %s. ", (*it).info.c_str(), repo->ID().c_str()); + return false; + } + reposum += dirsum; + } } - if (checksum != reposum || checksum.empty()) + if (oldReposum != reposum || oldReposum.empty()) { map uniqueAddons; for (CRepository::DirList::const_iterator it = repo->m_dirs.begin(); it != repo->m_dirs.end(); ++it) { if (ShouldCancel(0, 0)) - return addons; - VECADDONS addons2 = CRepository::Parse(*it); - MergeAddons(uniqueAddons, addons2); + return false; + VECADDONS addons; + if (!CRepository::Parse(*it, addons)) + { //TODO: Hash is invalid and should not be saved, but should we fail? + //We can still report a partial addon listing. + CLog::Log(LOGERROR, "Failed to read directory listing %s for repository %s. ", (*it).info.c_str(), repo->ID().c_str()); + return false; + } + MergeAddons(uniqueAddons, addons); } - if (uniqueAddons.empty()) + bool add = true; + if (!repo->Props().libname.empty()) { - CLog::Log(LOGERROR,"Repository %s returned no add-ons, listing may have failed",repo->Name().c_str()); - reposum = checksum; // don't update the checksum + CFileItemList dummy; + string s = StringUtils::Format("plugin://%s/?action=update", repo->ID().c_str()); + add = CDirectory::GetDirectory(s, dummy); } - else + if (add) { - bool add=true; - if (!repo->Props().libname.empty()) - { - CFileItemList dummy; - string s = StringUtils::Format("plugin://%s/?action=update", repo->ID().c_str()); - add = CDirectory::GetDirectory(s, dummy); - } - if (add) - { - for (map::const_iterator i = uniqueAddons.begin(); i != uniqueAddons.end(); ++i) - addons.push_back(i->second); - database.AddRepository(repo->ID(),addons,reposum); - } + for (map::const_iterator i = uniqueAddons.begin(); i != uniqueAddons.end(); ++i) + addons.push_back(i->second); + database.AddRepository(repo->ID(),addons,reposum); } } else - database.GetRepository(repo->ID(),addons); - database.SetRepoTimestamp(repo->ID(),CDateTime::GetCurrentDateTime().GetAsDBDateTime()); - - return addons; + { + CLog::Log(LOGDEBUG, "Checksum for repository %s not changed.", repo->ID().c_str()); + database.GetRepository(repo->ID(), addons); + database.SetRepoTimestamp(repo->ID(), CDateTime::GetCurrentDateTime().GetAsDBDateTime()); + } + return true; } diff --git a/xbmc/addons/Repository.h b/xbmc/addons/Repository.h index 7331713b77f4d..8ac282c6084bf 100644 --- a/xbmc/addons/Repository.h +++ b/xbmc/addons/Repository.h @@ -55,7 +55,7 @@ namespace ADDON typedef std::vector DirList; DirList m_dirs; - static VECADDONS Parse(const DirInfo& dir); + static bool Parse(const DirInfo& dir, VECADDONS& addons); static std::string FetchChecksum(const std::string& url); virtual void OnPostInstall(bool restart, bool update); @@ -74,7 +74,7 @@ namespace ADDON virtual const char *GetType() const { return "repoupdate"; }; virtual bool DoWork(); private: - VECADDONS GrabAddons(RepositoryPtr& repo); + bool GrabAddons(const RepositoryPtr& repo, VECADDONS& addons); VECADDONS m_repos; };