From 54cc69889ed056d93e416fa76c4d6a6b0ed30ebc Mon Sep 17 00:00:00 2001 From: "Jay Freeman (saurik)" Date: Sun, 29 Jan 2017 15:01:00 -0800 Subject: The entire concept of PendingError() is flawed :/. --- apt-pkg/cachefile.cc | 18 ++++----------- apt-pkg/contrib/error.cc | 9 ++++++++ apt-pkg/contrib/error.h | 20 ++++++++++++++++ apt-pkg/deb/debindexfile.cc | 6 ++--- apt-pkg/deb/deblistparser.cc | 2 +- apt-pkg/indexfile.cc | 2 +- apt-pkg/pkgcachegen.cc | 54 ++++++++++++++++++++++++-------------------- apt-pkg/policy.cc | 3 ++- 8 files changed, 70 insertions(+), 44 deletions(-) diff --git a/apt-pkg/cachefile.cc b/apt-pkg/cachefile.cc index 9a1a6cfa9..8168a9af3 100644 --- a/apt-pkg/cachefile.cc +++ b/apt-pkg/cachefile.cc @@ -97,7 +97,7 @@ bool pkgCacheFile::BuildCaches(OpProgress *Progress, bool WithLock) return false; Cache.reset(new pkgCache(Map.get())); if (_error->PendingError() == true) - return false; + return _error->ReturnError(); this->Cache = Cache.release(); this->Map = Map.release(); @@ -112,7 +112,7 @@ bool pkgCacheFile::BuildCaches(OpProgress *Progress, bool WithLock) } if (_error->PendingError() == true) - return false; + return _error->ReturnError(); if (BuildSourceList(Progress) == false) return false; @@ -128,14 +128,8 @@ bool pkgCacheFile::BuildCaches(OpProgress *Progress, bool WithLock) if (Res == false) return _error->Error(_("The package lists or status file could not be parsed or opened.")); - /* This sux, remove it someday */ - if (_error->PendingError() == true) - _error->Warning(_("You may want to run apt-get update to correct these problems")); - if (Cache == nullptr) Cache.reset(new pkgCache(Map.get())); - if (_error->PendingError() == true) - return false; this->Map = Map.release(); this->Cache = Cache.release(); @@ -169,7 +163,7 @@ bool pkgCacheFile::BuildPolicy(OpProgress * /*Progress*/) Policy.reset(new pkgPolicy(Cache)); if (_error->PendingError() == true) - return false; + return _error->ReturnError(); ReadPinFile(*Policy); ReadPinDir(*Policy); @@ -195,7 +189,7 @@ bool pkgCacheFile::BuildDepCache(OpProgress *Progress) DCache.reset(new pkgDepCache(Cache,Policy)); if (_error->PendingError() == true) - return false; + return _error->ReturnError(); if (DCache->Init(Progress) == false) return false; @@ -219,8 +213,6 @@ bool pkgCacheFile::Open(OpProgress *Progress, bool WithLock) if (Progress != NULL) Progress->Done(); - if (_error->PendingError() == true) - return false; return true; } @@ -266,7 +258,7 @@ bool pkgCacheFile::AddIndexFile(pkgIndexFile * const File) /*{{{*/ if (_error->PendingError() == true) { delete Cache; Cache = nullptr; - return false; + return _error->ReturnError(); } return true; } diff --git a/apt-pkg/contrib/error.cc b/apt-pkg/contrib/error.cc index 3c397eaf8..143b99557 100644 --- a/apt-pkg/contrib/error.cc +++ b/apt-pkg/contrib/error.cc @@ -206,6 +206,15 @@ void GlobalError::Discard() { PendingFlag = false; } /*}}}*/ +// GlobalError::ReturnError - convert a stored error to a return code /*{{{*/ +bool GlobalError::ReturnError() { + for (auto &message : Messages) + if (message.Type == ERROR) + message.Type = WARNING; + PendingFlag = false; + return false; +} + /*}}}*/ // GlobalError::empty - does our error list include anything? /*{{{*/ bool GlobalError::empty(MsgType const &threshold) const { if (PendingFlag == true) diff --git a/apt-pkg/contrib/error.h b/apt-pkg/contrib/error.h index d0f450742..bab7a67b2 100644 --- a/apt-pkg/contrib/error.h +++ b/apt-pkg/contrib/error.h @@ -226,6 +226,26 @@ public: /*{{{*/ */ inline bool PendingError() const APT_PURE {return PendingFlag;}; + /** \brief convert a stored error to a return code + * + * Put simply, the entire concept of PendingError() is flawed :/. + * + * The typical "if (PendingError()) return false;" check that is + * strewn throughout the codebase "compounds", making it impossible + * for there to be any nuance about the notion of "error" when a + * subsystem needs to fail but a higher-level system needs to work. + * + * However, the codebase is also horribly broken with respect to + * errors, as it fails to use C++ exceptions when warranted and + * instead relies on this insane indirect error mechanism to check + * the failure status of a constructor. What is thereby needed is + * a way to clear the PendingError() flag without also discarding + * the underlying errors, so we have to convert them to warnings. + * + * \return \b false + */ + bool ReturnError() APT_COLD; + /** \brief is the list empty? * * Can be used to check if the current stack level doesn't include diff --git a/apt-pkg/deb/debindexfile.cc b/apt-pkg/deb/debindexfile.cc index 25e0a3312..78e3eecb7 100644 --- a/apt-pkg/deb/debindexfile.cc +++ b/apt-pkg/deb/debindexfile.cc @@ -130,7 +130,7 @@ pkgCacheListParser * debTranslationsIndex::CreateListParser(FileFd &Pkg) std::unique_ptr Parser(new debTranslationsParser(&Pkg)); bool const newError = _error->PendingError(); _error->MergeWithStack(); - return newError ? nullptr : Parser.release(); + return newError ? ((void)_error->ReturnError(), nullptr) : Parser.release(); } /*}}}*/ // dpkg/status Index /*{{{*/ @@ -158,7 +158,7 @@ pkgCacheListParser * debStatusIndex::CreateListParser(FileFd &Pkg) std::unique_ptr Parser(new debStatusListParser(&Pkg)); bool const newError = _error->PendingError(); _error->MergeWithStack(); - return newError ? nullptr : Parser.release(); + return newError ? ((void)_error->ReturnError(), nullptr) : Parser.release(); } /*}}}*/ // DebPkgFile Index - a single .deb file as an index /*{{{*/ @@ -228,7 +228,7 @@ pkgCacheListParser * debDebPkgFileIndex::CreateListParser(FileFd &Pkg) std::unique_ptr Parser(new debDebFileParser(&Pkg, DebFile)); bool const newError = _error->PendingError(); _error->MergeWithStack(); - return newError ? nullptr : Parser.release(); + return newError ? ((void)_error->ReturnError(), nullptr) : Parser.release(); } uint8_t debDebPkgFileIndex::GetIndexFlags() const { diff --git a/apt-pkg/deb/deblistparser.cc b/apt-pkg/deb/deblistparser.cc index 68c3498c8..b737a9207 100644 --- a/apt-pkg/deb/deblistparser.cc +++ b/apt-pkg/deb/deblistparser.cc @@ -86,7 +86,7 @@ string debListParser::Package() { } if(unlikely(Result.empty() == true)) - _error->Error("Encountered a section with no Package: header"); + _error->Warning("Encountered a section with no Package: header"); return Result; } /*}}}*/ diff --git a/apt-pkg/indexfile.cc b/apt-pkg/indexfile.cc index 7574283f0..85d0e87cd 100644 --- a/apt-pkg/indexfile.cc +++ b/apt-pkg/indexfile.cc @@ -374,7 +374,7 @@ bool pkgDebianIndexFile::Merge(pkgCacheGenerator &Gen,OpProgress * const Prog) File->mtime = Pkg.ModificationTime(); if (Gen.MergeList(*Parser) == false) - return _error->Warning("Problem with MergeList %s",PackageFile.c_str()); + return _error->Error("Problem with MergeList %s",PackageFile.c_str()); return true; } pkgCache::PkgFileIterator pkgDebianIndexFile::FindInCache(pkgCache &Cache) const diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index 8b3f8550d..de724d7d5 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -68,7 +68,7 @@ bool pkgCacheGenerator::Start() bool const newError = _error->PendingError(); _error->MergeWithStack(); if (newError) - return false; + return _error->ReturnError(); if (Map.Size() <= 0) return false; @@ -132,7 +132,7 @@ bool pkgCacheGenerator::Start() advoid a problem during a crash */ pkgCacheGenerator::~pkgCacheGenerator() { - if (_error->PendingError() == true || Map.validData() == false) + if (Map.validData() == false) return; if (Map.Sync() == false) return; @@ -249,10 +249,8 @@ bool pkgCacheGenerator::MergeList(ListParser &List, while (List.Step() == true) { string const PackageName = List.Package(); - if (PackageName.empty() == true) { - _error->Warning("Encountered a section with no Package: header"); + if (PackageName.empty() == true) continue; - } Counter++; if (Counter % 100 == 0 && Progress != 0) @@ -276,7 +274,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List, if (NewPackage(Pkg, PackageName, Arch) == false) { // TRANSLATOR: The first placeholder is a package name, // the other two should be copied verbatim as they include debug info - _error->Warning(_("Error occurred while processing %s (%s%d)"), + _error->Error(_("Error occurred while processing %s (%s%d)"), PackageName.c_str(), "NewPackage", 1); continue; } @@ -340,7 +338,7 @@ bool pkgCacheGenerator::MergeListPackage(ListParser &List, pkgCache::PkgIterator pkgCache::VerIterator Ver(Cache); Dynamic DynVer(Ver); if (List.UsePackage(Pkg, Ver) == false) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "UsePackage", 1); // Find the right version to write the description @@ -419,11 +417,11 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator if (Res == 0 && Ver.end() == false && Ver->Hash == Hash) { if (List.UsePackage(Pkg,Ver) == false) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "UsePackage", 2); if (NewFileVer(Ver,List) == false) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "NewFileVer", 1); // Read only a single record and return @@ -440,7 +438,7 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator // Add a new version map_pointer_t const verindex = NewVersion(Ver, Version, Pkg.Index(), Hash, *LastVer); if (unlikely(verindex == 0)) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "NewVersion", 1); if (oldMap != Map.Data()) @@ -448,15 +446,15 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator *LastVer = verindex; if (unlikely(List.NewVersion(Ver) == false)) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "NewVersion", 2); if (unlikely(List.UsePackage(Pkg,Ver) == false)) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "UsePackage", 3); if (unlikely(NewFileVer(Ver,List) == false)) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "NewFileVer", 2); pkgCache::GrpIterator Grp = Pkg.Group(); @@ -477,12 +475,12 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator Dynamic DynV(V); for (; V.end() != true; ++V) if (unlikely(AddImplicitDepends(V, Pkg) == false)) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "AddImplicitDepends", 1); } } if (unlikely(AddImplicitDepends(Grp, Pkg, Ver) == false)) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "AddImplicitDepends", 2); // Read only a single record and return @@ -526,7 +524,7 @@ bool pkgCacheGenerator::AddNewDescription(ListParser &List, pkgCache::VerIterato map_pointer_t const descindex = NewDescription(Desc, lang, CurMd5, md5idx); if (unlikely(descindex == 0)) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Ver.ParentPkg().Name(), "NewDescription", 1); md5idx = Desc->md5sum; @@ -540,7 +538,7 @@ bool pkgCacheGenerator::AddNewDescription(ListParser &List, pkgCache::VerIterato *LastNextDesc = descindex; if (NewFileDesc(Desc,List) == false) - return _error->Warning(_("Error occurred while processing %s (%s%d)"), + return _error->Error(_("Error occurred while processing %s (%s%d)"), Ver.ParentPkg().Name(), "NewFileDesc", 1); return true; @@ -1435,7 +1433,7 @@ static bool CheckValidity(FileFd &CacheFile, std::string const &CacheFileName, { if (Debug == true) std::clog << "Errors are pending or Map is empty() for " << CacheFileName << std::endl; - return false; + return _error->ReturnError(); } std::unique_ptr RlsVisited(new bool[Cache.HeaderP->ReleaseFileCount]); @@ -1515,7 +1513,7 @@ static bool CheckValidity(FileFd &CacheFile, std::string const &CacheFileName, std::clog << "Validity failed because of pending errors:" << std::endl; _error->DumpErrors(std::clog, GlobalError::DEBUG, false); } - return false; + return _error->ReturnError(); } if (OutMap != 0) @@ -1578,8 +1576,10 @@ static void BuildCache(pkgCacheGenerator &Gen, Progress->OverallProgress(CurrentSize, TotalSize, Size, _("Reading package lists")); CurrentSize += Size; - if (I->Merge(Gen,Progress) == false) + if (I->Merge(Gen,Progress) == false) { + _error->ReturnError(); return; + } }; if (List != NULL) @@ -1593,8 +1593,10 @@ static void BuildCache(pkgCacheGenerator &Gen, continue; } - if ((*i)->Merge(Gen, Progress) == false) + if ((*i)->Merge(Gen, Progress) == false) { + _error->ReturnError(); continue; + } std::vector *Indexes = (*i)->GetIndexFiles(); if (Indexes != NULL) @@ -1665,7 +1667,7 @@ static bool loadBackMMapFromFile(std::unique_ptr &Gen, bool const newError = _error->PendingError(); _error->MergeWithStack(); if (alloc == 0 && newError) - return false; + return _error->ReturnError(); if (CacheF.Read((unsigned char *)Map->Data() + alloc, CacheF.Size()) == false) return false; Gen.reset(new pkgCacheGenerator(Map.get(),Progress)); @@ -1853,13 +1855,15 @@ bool pkgCacheGenerator::MakeOnlyStatusCache(OpProgress *Progress,DynamicMMap **O if (Progress != NULL) Progress->OverallProgress(0,1,1,_("Reading package lists")); pkgCacheGenerator Gen(Map.get(),Progress); - if (Gen.Start() == false || _error->PendingError() == true) + if (Gen.Start() == false) return false; + if (_error->PendingError() == true) + return _error->ReturnError(); BuildCache(Gen,Progress,CurrentSize,TotalSize, NULL, Files.begin(), Files.end()); + // We've passed the point of no return + _error->ReturnError(); - if (_error->PendingError() == true) - return false; *OutMap = Map.release(); return true; diff --git a/apt-pkg/policy.cc b/apt-pkg/policy.cc index 7986aa506..614539266 100644 --- a/apt-pkg/policy.cc +++ b/apt-pkg/policy.cc @@ -333,7 +333,7 @@ bool ReadPinDir(pkgPolicy &Plcy,string Dir) bool const PendingErrors = _error->PendingError(); _error->MergeWithStack(); if (PendingErrors) - return false; + return _error->ReturnError(); // Read the files bool good = true; @@ -415,6 +415,7 @@ bool ReadPinFile(pkgPolicy &Plcy,string File) if (priority < std::numeric_limits::min() || priority > std::numeric_limits::max() || newError) { + _error->ReturnError(); return _error->Error(_("%s: Value %s is outside the range of valid pin priorities (%d to %d)"), File.c_str(), Tags.FindS("Pin-Priority").c_str(), std::numeric_limits::min(), -- cgit v1.2.3