From baec76f5f0f9fcbd71f6e2afaa7fc85543bd624c Mon Sep 17 00:00:00 2001 From: "Jay Freeman (saurik)" Date: Sun, 29 Jan 2017 15:01:00 -0800 Subject: [PATCH] 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 | 3 ++ apt-pkg/deb/deblistparser.cc | 2 +- apt-pkg/indexfile.cc | 3 +- apt-pkg/pkgcachegen.cc | 54 +++++++++++++++++++----------------- apt-pkg/policy.cc | 3 +- 8 files changed, 71 insertions(+), 41 deletions(-) diff --git a/apt-pkg/cachefile.cc b/apt-pkg/cachefile.cc index b5f32fc29..90f803ad6 100644 --- a/apt-pkg/cachefile.cc +++ b/apt-pkg/cachefile.cc @@ -90,7 +90,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(); @@ -102,7 +102,7 @@ bool pkgCacheFile::BuildCaches(OpProgress *Progress, bool WithLock) return false; if (_error->PendingError() == true) - return false; + return _error->ReturnError(); if (BuildSourceList(Progress) == false) return false; @@ -118,14 +118,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(); @@ -159,7 +153,7 @@ bool pkgCacheFile::BuildPolicy(OpProgress * /*Progress*/) Policy.reset(new pkgPolicy(Cache)); if (_error->PendingError() == true) - return false; + return _error->ReturnError(); if (ReadPinFile(*Policy) == false || ReadPinDir(*Policy) == false) return false; @@ -185,7 +179,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; @@ -209,8 +203,6 @@ bool pkgCacheFile::Open(OpProgress *Progress, bool WithLock) if (Progress != NULL) Progress->Done(); - if (_error->PendingError() == true) - return false; return true; } @@ -256,7 +248,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 c06ea8364..7d397d2c6 100644 --- a/apt-pkg/contrib/error.cc +++ b/apt-pkg/contrib/error.cc @@ -227,6 +227,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 e56999b14..bcaa7c995 100644 --- a/apt-pkg/contrib/error.h +++ b/apt-pkg/contrib/error.h @@ -227,6 +227,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 c55847305..6b162372d 100644 --- a/apt-pkg/deb/debindexfile.cc +++ b/apt-pkg/deb/debindexfile.cc @@ -134,6 +134,7 @@ pkgCacheListParser * debTranslationsIndex::CreateListParser(FileFd &Pkg) if (newError) { delete Parser; + _error->ReturnError(); return nullptr; } else @@ -168,6 +169,7 @@ pkgCacheListParser * debStatusIndex::CreateListParser(FileFd &Pkg) if (newError) { delete Parser; + _error->ReturnError(); return nullptr; } else @@ -250,6 +252,7 @@ pkgCacheListParser * debDebPkgFileIndex::CreateListParser(FileFd &Pkg) if (newError) { delete Parser; + _error->ReturnError(); return nullptr; } else diff --git a/apt-pkg/deb/deblistparser.cc b/apt-pkg/deb/deblistparser.cc index 25d8e6f22..23048008b 100644 --- a/apt-pkg/deb/deblistparser.cc +++ b/apt-pkg/deb/deblistparser.cc @@ -76,7 +76,7 @@ string debListParser::Package() { std::transform(Result.begin(), Result.end(), Result.begin(), tolower_ascii); 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 74d46d699..21765388f 100644 --- a/apt-pkg/indexfile.cc +++ b/apt-pkg/indexfile.cc @@ -344,6 +344,7 @@ pkgCacheListParser * pkgDebianIndexFile::CreateListParser(FileFd &Pkg) if (newError) { delete Parser; + _error->ReturnError(); return nullptr; } else @@ -377,7 +378,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 51bb9ec7b..8ee682db8 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -70,7 +70,7 @@ bool pkgCacheGenerator::Start() bool const newError = _error->PendingError(); _error->MergeWithStack(); if (newError) - return false; + return _error->ReturnError(); if (Map.Size() <= 0) return false; @@ -134,7 +134,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; @@ -1432,7 +1430,7 @@ static bool CheckValidity(const string &CacheFile, { if (Debug == true) std::clog << "Errors are pending or Map is empty() for " << CacheFile << std::endl; - return false; + return _error->ReturnError(); } std::unique_ptr RlsVisited(new bool[Cache.HeaderP->ReleaseFileCount]); @@ -1512,7 +1510,7 @@ static bool CheckValidity(const string &CacheFile, 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) @@ -1575,8 +1573,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) @@ -1590,8 +1590,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) @@ -1663,7 +1665,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)); @@ -1849,13 +1851,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 3dd6ddac4..a13510e66 100644 --- a/apt-pkg/policy.cc +++ b/apt-pkg/policy.cc @@ -320,7 +320,7 @@ bool ReadPinDir(pkgPolicy &Plcy,string Dir) bool const PendingErrors = _error->PendingError(); _error->MergeWithStack(); if (PendingErrors) - return false; + return _error->ReturnError(); // Read the files for (vector::const_iterator I = List.begin(); I != List.end(); ++I) @@ -391,6 +391,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(), -- 2.45.2