Система: alt-education-9.0-i586.iso со средой рабочего стола kde. В разделе Обновления при нажатии на "Обновить все" происходит сбой службы PackageKit (скриншот 1.png). Затем всплывает ещё одна ошибка (скриншот 2.png).
Created attachment 8397 [details] 1.png
Created attachment 8398 [details] 2.png
Ошибка все еще воспроизводится plasma5-discover-5.17.4-alt2 Актуально только для i586
А актуальна ли вообще починка packagekit для i586. Хотя, оно и в apt уходить может.
Проблема воспроизвелась на Сизифе i586. Похоже, что дело оказалось в утечке памяти в apt, в результате которой packagekit исчерпывает всё доступное для 32-битного процесса адресное пространство и в итоге падает. Исправление для Сизифа доступно в задании #240120.
Спасибо! В 0.5.15lorg2-alt72 (в Sisyphus) исправлены некоторые утечки памяти и в самом apt [1] [3], и проявлявшиеся при "неправильном" использовании API apt из packagekit [2]. Before [1], DynamicMMap's underlying file was extended unnecessarily by one byte. Contrary to the behavior before [2], packagekit assumed that pkgCacheFile::BuildCaches() is a kind of idempotent action (never rebuilding something if it exists); but our apt had an old (incorrect?) semantics for it. So, this interaction simply caused huge memory leaks (and potentially dangling pointers in packagekit). [2] changed the behavior to the modern one, and fixed the leaks. Assigning to *OutMap (gone in [3]) in the pkgMakeStatusCache() and similar functions in pkgcachegen.cc could potentially lead to a leak of the pointer already stored in *OutMap. However, after the way of using these functions in cachefile.cc changed in [2], this has not been an actual problem, because the new pointer has not been anymore directly overwriting an old value, but rather this has been controlled by lazyCacheFile::BuildMap(), which wouldn't overwrite an existing pointer, and intermediately the pointer has been stored in a unique_ptr in pkgCacheFile::MakeMap(). It's anyway better to have an API (the new pkgMakeStatusCache() and similar functions in pkgcachegen.cc) that prevents potential misuse and leaks (also in other places than cachefile.cc, say, in apt-cache.cc). [1]: http://git.altlinux.org/gears/a/apt.git?p=apt.git;a=commitdiff;h=3462764ec85e05b77a7e3ca0cc884139fc96853f commit 3462764ec85e05b77a7e3ca0cc884139fc96853f Author: Dmitry V. Levin <ldv@altlinux.org> Date: Fri Feb 14 20:53:59 2020 +0000 DynamicMMap: fix resource leak Every time DynamicMMap object was created with WorkSpace equal to the size of the underlying file, the latter was extended unnecessarily by one byte. Reported-by: Aleksei Nikiforov <darktemplar@altlinux.org> Reported-by: Ivan Zakharyaschev <imz@altlinux.org> Resolves: https://bugzilla.altlinux.org/37481 diff --git a/apt-pkg/contrib/mmap.cc b/apt-pkg/contrib/mmap.cc index 36643fd58..d63142606 100644 --- a/apt-pkg/contrib/mmap.cc +++ b/apt-pkg/contrib/mmap.cc @@ -176,20 +176,19 @@ DynamicMMap::DynamicMMap(FileFd &F, for new allocations and holding already allocated stuff. */ - unsigned long EndOfFile = Fd->Size(); + auto EndOfFile = Fd->Size(); if (EndOfFile > WorkSpace) /* The already allocated and saved stuff exceeds the requested workspace, so workspace must be increased. */ WorkSpace = EndOfFile; - else + else if (EndOfFile < WorkSpace) { /* The backing file must be made at least as big as the workspace that we are going to use in the course of work. */ - Fd->Seek(WorkSpace); - char C = 0; - Fd->Write(&C,sizeof(C)); + if (!Fd->Truncate(WorkSpace)) + return; } Map(F); /* sets iSize to the size of the whole, possibly extended file */ [2]: http://git.altlinux.org/gears/a/apt.git?p=apt.git;a=commitdiff;h=0193084a08fe443f9ba82b9f4c91daf01964df90 commit 0193084a08fe443f9ba82b9f4c91daf01964df90 Author: Ivan Zakharyaschev <imz@altlinux.org> Date: Fri Jun 11 09:49:27 2021 +0300 derive pkgCacheFile from a new class which imposes laziness and immutability This change (and a few previous commits) was conceived after an investigation of a crash in PackageKit lead to the understanding that PackageKit expects the current Debian's semantics of pkgCacheFile::BuildCaches() API (incl. all methods of this kind that they have) which differs from what we had in the old APT in ALT. According to those expectation, BuildCaches() should be a kind of idempotent action: if the owned object is built already, it shouldn't replace it with a new one. (Allowing to replace an owned object was bad if used inappropriately: the dependent objects which have a pointer to it would be spoilt; the old object would leak, etc.) So the new Debian's presentation of pkgCacheFile as a kind of lazy immutable structure (whose members are built only once, on demand) makes sense. (To be more precise, the resulting "monotonicity" is a good thing: the members can only become more complete, but never get replaced. As for the "laziness", that's not obviously good, because the seemingly convenient accessors like GetPkgCache() are not actually safe unless you always check for errors/NULL: the programmer can't be sure whether they could be computed successfully or a runtime error happened. That's a drawback for which we don't have a solution as for now.) I tried to make this notion more formal (expressed by the constraints in code, so that it's more difficult for derived code to violate it). And so my change was more radical than what we see in Debian. The immutability principle, however, could be broken by Close(), which I deleted from the API. This change can be seen as a move towards RAII paradigm ("resource acquisition is initialization") w.r.t. the pkgCacheFile class. Having Close() and Open() (which do the work that under RAII just the dtor and ctor are supposed to do) breaks RAII. For now, I haven't deleted Open() (but have deleted Close()) from the API--not to do too much work. But I moved a parameter from Open() to the ctor in order to move in the direction of real RAII (n future, perhaps). For reference, Debian's changes in this respect: commit 2e5f4e45f593535e2c88181ff7a9e2d32a5e60f9 Author: David Kalnischkies <kalnischkies@gmail.com> Date: Fri Jun 4 14:43:03 2010 +0200 * apt-pkg/cachefile.{cc,h}: - split Open() into submethods to be able to build only parts - make the OpProgress optional in the Cache buildprocess commit 3f8621c5effb167a51944b86334867e2fe8fedb3 Author: David Kalnischkies <kalnischkies@gmail.com> Date: Fri Jun 4 22:58:57 2010 +0200 store also the SourceList we use internally for export (It's not clear from the message, but it's commit 3f8621c5effb167a51944b86334867e2fe8fedb3 that introduced the idempotent semantics to BuildCache() and the kin, so that nothing bad happens if they are called many times, as in PackageKit. Probably, at that time, no one did that really, so it wasn't a big noticeable change.) commit f40fdaa43271edf98b80c08e20f401b5da591501 Author: Julian Andres Klode <jak@debian.org> Date: Sat Mar 19 01:56:38 2016 +0100 cachefile: Only set members that were initialized successfully Otherwise, things will just start failing later down the stack, because (a) the lazy getters do not check if building was successful and (b) any further getter call would return the invalid object anyway. Also initialize VS in pkgCache to nullptr by default. Closes: #818628 This issue was also already acknowledged before by a suggested series of patches to ALT's APT: commit a1ddf65fa87a24622c4ff4c250c3c997d408611d Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Mon Oct 28 15:01:18 2019 +0300 Fix resource leaks in pkgCacheFile class diff --git a/apt/apt-pkg/cachefile.cc b/apt/apt-pkg/cachefile.cc index 1a5fa64c6..0ac882bbd 100644 --- a/apt/apt-pkg/cachefile.cc +++ b/apt/apt-pkg/cachefile.cc @@ -81,6 +81,12 @@ bool pkgCacheFile::BuildCaches(OpProgress &Progress,bool WithLock) return false; // Read the caches + if (Map != nullptr) + { + delete Map; + Map = nullptr; + } + bool Res = pkgMakeStatusCache(*SrcList,Progress,&Map,!WithLock); Progress.Done(); if (Res == false) @@ -90,6 +96,12 @@ bool pkgCacheFile::BuildCaches(OpProgress &Progress,bool WithLock) if (_error->empty() == false) _error->Warning(_("You may want to run apt-get update to correct these problems")); + if (Cache != nullptr) + { + delete Cache; + Cache = nullptr; + } + Cache = new pkgCache(Map); if (_error->PendingError() == true) return false; diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc index 37364e91e..2109c851f 100644 --- a/apt/apt-pkg/pkgcachegen.cc +++ b/apt/apt-pkg/pkgcachegen.cc @@ -835,7 +835,14 @@ static bool CheckValidity(const string &CacheFile, FileIterator Start, } if (OutMap != 0) + { + if (*OutMap != nullptr) + { + delete *OutMap; + } + *OutMap = Map.UnGuard(); + } return true; } /*}}}*/ @@ -1131,6 +1138,11 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, return false; if (OutMap != 0) { + if (*OutMap != nullptr) + { + delete *OutMap; + } + if (CacheF != 0) { delete Map.UnGuard(); commit 5abad53b7846054bef6464ef3dac6cda2e08bf45 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Mon Oct 28 16:57:48 2019 +0300 pkgCacheFile: don't regenerate cache if it was already built diff --git a/apt/apt-pkg/cachefile.cc b/apt/apt-pkg/cachefile.cc index 8b17345f9..41ca057ef 100644 --- a/apt/apt-pkg/cachefile.cc +++ b/apt/apt-pkg/cachefile.cc @@ -58,6 +58,10 @@ pkgCacheFile::~pkgCacheFile() /* */ bool pkgCacheFile::BuildCaches(OpProgress &Progress,bool WithLock) { + // Reuse cache if possible + if (this->Cache != NULL) + return true; + if (WithLock == true) if (_system->Lock() == false) return false; diff --git a/apt-pkg/cachefile.cc b/apt-pkg/cachefile.cc index 76bb0c64b..64c278b70 100644 --- a/apt-pkg/cachefile.cc +++ b/apt-pkg/cachefile.cc @@ -34,12 +34,11 @@ #include <apti18n.h> /*}}}*/ -// CacheFile::CacheFile - Constructor /*{{{*/ +// lazyCacheFile::lazyCacheFile - Constructor /*{{{*/ // --------------------------------------------------------------------- /* */ -pkgCacheFile::pkgCacheFile(const bool WithLock) - : WithLock(WithLock) - , Lock(false) +lazyCacheFile::lazyCacheFile() + : Lock(false) , SrcList(nullptr) , Map(nullptr) , Cache(nullptr) @@ -48,10 +47,10 @@ pkgCacheFile::pkgCacheFile(const bool WithLock) { } /*}}}*/ -// CacheFile::~CacheFile - Destructor /*{{{*/ +// lazyCacheFile::~lazyCacheFile - Destructor /*{{{*/ // --------------------------------------------------------------------- /* */ -pkgCacheFile::~pkgCacheFile() +lazyCacheFile::~lazyCacheFile() { delete DCache; delete Policy; @@ -69,57 +68,152 @@ pkgCacheFile::~pkgCacheFile() DCache = nullptr; } /*}}}*/ -// CacheFile::BuildCaches - Open and build the cache files /*{{{*/ +// These methods build the members and never recreate anything that exists. + +bool lazyCacheFile::BuildLock() +{ + return Lock || (Lock = MakeLock()); +} + +bool lazyCacheFile::BuildMap(OpProgress &Progress) { + return Map || (Map = MakeMap(Progress).release()); +} + +bool lazyCacheFile::BuildCaches(OpProgress &Progress) +{ + return Cache || (Cache = MakePkgCache(Progress).release()); +} + +bool lazyCacheFile::BuildSourceList(OpProgress *Progress) +{ + return SrcList || (SrcList = MakeSrcList(Progress).release()); +} + +bool lazyCacheFile::BuildPolicy(OpProgress &Progress) +{ + return Policy || (Policy = MakePolicy(Progress).release()); +} + +bool lazyCacheFile::BuildDepCache(OpProgress &Progress) +{ + return DCache || (DCache = MakeDepCache(Progress).release()); +} + +// pkgCacheFile::pkgCacheFile - Constructor /*{{{*/ // --------------------------------------------------------------------- /* */ -bool pkgCacheFile::BuildCaches(OpProgress &Progress) +pkgCacheFile::pkgCacheFile(const bool WithLock) + : lazyCacheFile() + , WithLock(WithLock) +{ +} + /*}}}*/ +bool pkgCacheFile::MakeLock() { + bool Lock = false; if (WithLock == true) Lock = _system->Lock(); else // CNC:2002-07-06 Lock = _system->LockRead(); - if (!Lock) - return false; if (_error->PendingError() == true) return false; + return Lock; +} + +std::unique_ptr<MMap> pkgCacheFile::MakeMap(OpProgress &Progress) +{ + if (!GetLock()) + return nullptr; // Read the source list - if (!BuildSourceList(&Progress)) - return false; + pkgSourceList * const SrcList = GetSourceList(); + if (!SrcList) + return nullptr; + + const bool AllowMem = (!WithLock + || _config->FindB("Debug::NoLocking",false)); + MMap * TmpMap = nullptr; + const bool Res = pkgMakeStatusCache(*SrcList,Progress,&TmpMap,AllowMem); + std::unique_ptr<MMap> Map(TmpMap); + Progress.Done(); + if (Res == false) + { + _error->Error(_("The package lists or status file could not be parsed or opened.")); + return nullptr; + } + /* This sux, remove it someday */ + if (_error->empty() == false) + _error->Warning(_("You may want to run apt-get update to correct these problems")); + return Map; +} + +// pkgCacheFile::MakePkgCache - Open and build the cache files /*{{{*/ +// --------------------------------------------------------------------- +/* */ +std::unique_ptr<pkgCache> pkgCacheFile::MakePkgCache(OpProgress &Progress) +{ // Read the caches + MMap * const Map = GetMap(Progress); + if (!Map) + return nullptr; + + std::unique_ptr<pkgCache> Cache(new pkgCache(Map)); + if (_error->PendingError() == true) + return nullptr; + return Cache; +} + /*}}}*/ +std::unique_ptr<pkgSourceList> pkgCacheFile::MakeSrcList(OpProgress * /*Progress*/) +{ + std::unique_ptr<pkgSourceList> SrcList(new pkgSourceList()); + if (!SrcList->ReadMainList()) { - const bool AllowMem = (!WithLock - || _config->FindB("Debug::NoLocking",false)); - const bool Res = pkgMakeStatusCache(*SrcList,Progress,&Map,AllowMem); - Progress.Done(); - 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->empty() == false) - _error->Warning(_("You may want to run apt-get update to correct these problems")); + _error->Error(_("The list of sources could not be read.")); + return nullptr; } - Cache = new pkgCache(Map); + return SrcList; +} + +// The policy engine +std::unique_ptr<pkgPolicy> pkgCacheFile::MakePolicy(OpProgress &Progress) +{ + pkgCache * const Cache = GetPkgCache(Progress); + if (!Cache) + return nullptr; + + std::unique_ptr<pkgPolicy> Policy(new pkgPolicy(Cache)); if (_error->PendingError() == true) - return false; - return true; + return nullptr; + if (ReadPinFile(*Policy) == false || ReadPinDir(*Policy) == false) + return nullptr; + + return Policy; } - /*}}}*/ -bool pkgCacheFile::BuildSourceList(OpProgress * /*Progress*/) + +// Create the dependency cache +std::unique_ptr<pkgDepCache> pkgCacheFile::MakeDepCache(OpProgress &Progress) { - std::unique_ptr<pkgSourceList> tmpSrcList; - if (SrcList != NULL) - return true; + pkgCache * const Cache = GetPkgCache(Progress); + if (!Cache) + return nullptr; - tmpSrcList.reset(new pkgSourceList()); - if (!tmpSrcList->ReadMainList()) - return _error->Error(_("The list of sources could not be read.")); - SrcList = tmpSrcList.release(); + pkgPolicy * const Policy = GetPolicy(Progress); + if (!Policy) + return nullptr; - return true; + std::unique_ptr<pkgDepCache> DCache(new pkgDepCache(Cache,Policy)); + if (_error->PendingError() == true) + return nullptr; + + if (DCache->Init(&Progress) == false) + return nullptr; + + return DCache; } + // CacheFile::Open - Open the cache files, creating if necessary /*{{{*/ // --------------------------------------------------------------------- /* */ @@ -128,19 +222,12 @@ bool pkgCacheFile::Open(OpProgress &Progress) if (BuildCaches(Progress) == false) return false; - // The policy engine - Policy = new pkgPolicy(Cache); - if (_error->PendingError() == true) - return false; - if (ReadPinFile(*Policy) == false || ReadPinDir(*Policy) == false) + if (BuildPolicy(Progress) == false) return false; - // Create the dependency cache - DCache = new pkgDepCache(Cache,Policy); - if (_error->PendingError() == true) + if (BuildDepCache(Progress) == false) return false; - DCache->Init(&Progress); Progress.Done(); if (_error->PendingError() == true) return false; diff --git a/apt-pkg/cachefile.h b/apt-pkg/cachefile.h index d367c38b8..a6a589e86 100644 --- a/apt-pkg/cachefile.h +++ b/apt-pkg/cachefile.h @@ -24,12 +24,19 @@ class pkgPolicy; class pkgSourceList; -class pkgCacheFile +/* An abstract class that owns pkgCache & its dependents (and some requisites). + It formally imposes the property (on any derived class) that once an owned + resource is "built" it doesn't ever change. + + Internally, it's lazy: the owned resources are built on demand. + + From outside, it looks like an immutable (lazy) structure. + */ + +class lazyCacheFile { protected: - const bool WithLock; - bool Lock; pkgSourceList *SrcList; MMap *Map; @@ -39,6 +46,58 @@ class pkgCacheFile public: + // These methods demand the resources; so, if needed, they are built. + // They are not safe; dangerous to use if you don't check for + // runtime errors/NULL return values. Regrettably... + inline bool GetLock() { BuildLock(); return Lock; } + inline MMap* GetMap(OpProgress &Progress) { BuildMap(Progress); return Map; } + inline pkgCache* GetPkgCache(OpProgress &Progress) { BuildCaches(Progress); return Cache; } + inline pkgDepCache* GetDepCache(OpProgress &Progress) { BuildDepCache(Progress); return DCache; } + inline pkgPolicy* GetPolicy(OpProgress &Progress) { BuildPolicy(Progress); return Policy; } + inline pkgSourceList* GetSourceList() { BuildSourceList(); return SrcList; } + + // These methods build the members and never recreate anything that exists. + bool BuildLock(); + bool BuildMap(OpProgress &Progress); + bool BuildCaches(OpProgress &Progress); + bool BuildSourceList(OpProgress *Progress = NULL); + bool BuildPolicy(OpProgress &Progress); + bool BuildDepCache(OpProgress &Progress); + + inline bool IsLockBuilt() const { return Lock; } + inline bool IsMapBuilt() const { return (Map != NULL); } + inline bool IsPkgCacheBuilt() const { return (Cache != NULL); } + inline bool IsDepCacheBuilt() const { return (DCache != NULL); } + inline bool IsPolicyBuilt() const { return (Policy != NULL); } + inline bool IsSrcListBuilt() const { return (SrcList != NULL); } + + lazyCacheFile(); + virtual ~lazyCacheFile(); + /* The owner must be unique. + + Prohibit copying (just in case the compiler doesn't see that + it's bad to implicitly define copy ctor and assignment for this class). + */ + lazyCacheFile(const lazyCacheFile &) = delete; + lazyCacheFile & operator=(const lazyCacheFile &) = delete; + + // These methods are implemented by subclasses. They can't break our rule. + virtual bool MakeLock() = 0; + virtual std::unique_ptr<MMap> MakeMap(OpProgress &Progress) = 0; + virtual std::unique_ptr<pkgCache> MakePkgCache(OpProgress &Progress) = 0; + virtual std::unique_ptr<pkgDepCache> MakeDepCache(OpProgress &Progress) = 0; + virtual std::unique_ptr<pkgPolicy> MakePolicy(OpProgress &Progress) = 0; + virtual std::unique_ptr<pkgSourceList> MakeSrcList(OpProgress *Progress) = 0; +}; + +class pkgCacheFile: public lazyCacheFile +{ + protected: + + const bool WithLock; + + public: + // We look pretty much exactly like a pointer to a dep cache inline operator pkgCache &() const {return *Cache;} inline operator pkgCache *() const {return Cache;} @@ -51,17 +110,17 @@ class pkgCacheFile inline pkgDepCache::StateCache &operator [](pkgCache::PkgIterator const &I) const {return (*DCache)[I];} inline unsigned char &operator [](pkgCache::DepIterator const &I) const {return (*DCache)[I];} - bool BuildCaches(OpProgress &Progress); - bool BuildSourceList(OpProgress *Progress = NULL); + bool MakeLock() override; + std::unique_ptr<MMap> MakeMap(OpProgress &Progress) override; + std::unique_ptr<pkgCache> MakePkgCache(OpProgress &Progress) override; + std::unique_ptr<pkgDepCache> MakeDepCache(OpProgress &Progress) override; + std::unique_ptr<pkgPolicy> MakePolicy(OpProgress &Progress) override; + std::unique_ptr<pkgSourceList> MakeSrcList(OpProgress *Progress) override; + bool Open(OpProgress &Progress); static void RemoveCaches(); - inline pkgSourceList* GetSourceList() { BuildSourceList(); return SrcList; } - - inline bool IsSrcListBuilt() const { return (SrcList != NULL); } - explicit pkgCacheFile(bool WithLock); - virtual ~pkgCacheFile(); }; // Helper for apt-get and apt-mark [3]: http://git.altlinux.org/gears/a/apt.git?p=apt.git;a=commitdiff;h=2d69b170da499419ba24ab092ed85ee3ae0c82fb commit 2d69b170da499419ba24ab092ed85ee3ae0c82fb Author: Ivan Zakharyaschev <imz@altlinux.org> Date: Fri Jan 22 05:51:40 2021 +0300 calling convention: if a function must construct an MMap on success, just return it (Affects ABI) The old calling convention for such functions was cumbersome: since they reported success as the bool result, they had to have an extra output parameter for the constructed MMap. Now we express more clearly that we construct a new object and the calling code is responsible for managing it (e.g., deleting) by returning a unique_ptr. And we also make it impossible to erroneously report success without actually constructing the MMap. (If the calling code just wants to check for success, it can simply discard the unique_ptr, and the object will be automatically deleted.) diff --git a/apt-pkg/cachefile.cc b/apt-pkg/cachefile.cc index 1a754b22b..f81c26a40 100644 --- a/apt-pkg/cachefile.cc +++ b/apt-pkg/cachefile.cc @@ -133,11 +133,9 @@ std::unique_ptr<MMap> pkgCacheFile::MakeMap(OpProgress &Progress) const bool AllowMem = (!WithLock || _config->FindB("Debug::NoLocking",false)); - MMap * TmpMap = nullptr; - const bool Res = pkgMakeStatusCache(*SrcList,Progress,&TmpMap,AllowMem); - std::unique_ptr<MMap> Map(TmpMap); + std::unique_ptr<MMap> Map = pkgMakeStatusCache(*SrcList,Progress,AllowMem); Progress.Done(); - if (Res == false) + if (Map == nullptr) { _error->Error(_("The package lists or status file could not be parsed or opened.")); return nullptr; diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index abc988b6a..90d0ae201 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -641,13 +641,13 @@ unsigned long pkgCacheGenerator::WriteUniqString(const char *S, // --------------------------------------------------------------------- /* This just verifies that each file in the list of index files exists, has matching attributes with the cache and the cache does not have - any extra files. */ -static bool CheckValidity(string CacheFile, FileIterator Start, - FileIterator End,MMap **OutMap = 0) + any extra files. On success, passes a non-null MMap as the returned value. */ +static std::unique_ptr<MMap> CheckValidity(string CacheFile, FileIterator Start, + FileIterator End) { // No file, certainly invalid if (CacheFile.empty() == true || FileExists(CacheFile) == false) - return false; + return nullptr; // CNC:2003-02-20 - When --reinstall is used during a cache building // process, the algorithm is sligthly changed to @@ -656,7 +656,9 @@ static bool CheckValidity(string CacheFile, FileIterator Start, // the cache when it's used. bool ReInstall = _config->FindB("APT::Get::ReInstall", false); if (ReInstall == true) - return false; + { + return nullptr; + } // Map it FileFd CacheF(CacheFile,FileFd::ReadOnly); @@ -665,12 +667,14 @@ static bool CheckValidity(string CacheFile, FileIterator Start, if (_error->PendingError() == true || Map->Size() == 0) { _error->Discard(); - return false; + return nullptr; } // CNC:2003-11-24 if (_system->OptionsHash() != Cache.HeaderP->OptionsHash) - return false; + { + return nullptr; + } /* Now we check every index file, see if it is in the cache, verify the IMS data and check that it is on the disk too.. */ @@ -692,24 +696,26 @@ static bool CheckValidity(string CacheFile, FileIterator Start, // FindInCache is also expected to do an IMS check. pkgCache::PkgFileIterator File = (*Start)->FindInCache(Cache); if (File.end() == true) - return false; + { + return nullptr; + } Visited[File->ID] = true; } for (unsigned I = 0; I != Cache.HeaderP->PackageFileCount; I++) if (Visited[I] == false) - return false; + { + return nullptr; + } if (_error->PendingError() == true) { _error->Discard(); - return false; + return nullptr; } - if (OutMap != 0) - *OutMap = Map.release(); - return true; + return Map; } /*}}}*/ // ComputeSize - Compute the total size of a bunch of files /*{{{*/ @@ -793,19 +799,20 @@ static bool CollectFileProvides(pkgCacheGenerator &Gen, // --------------------------------------------------------------------- /* This makes sure that the status cache (the cache that has all index files from the sources list and all local ones) is ready - to be mmaped. If OutMap is not zero then a MMap object representing - the cache will be stored there. This is pretty much mandatory if you + to be mmaped. On success, returns a non-null pointer to a MMap object + representing the cache, which the caller can keep or discard (the ownership + is transferred to the caller). Keeping it is pretty much mandatory if you are using AllowMem. AllowMem lets the function be run as non-root where it builds the cache 'fast' into a memory buffer. */ -bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, - MMap **OutMap,bool AllowMem) +std::unique_ptr<MMap> pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, + bool AllowMem) { unsigned long MapSize = _config->FindI("APT::Cache-Limit",6*(16+sizeof(long))*1024*1024); vector<pkgIndexFile *> Files(List.begin(),List.end()); unsigned long EndOfSource = Files.size(); if (_system->AddStatusFiles(Files) == false) - return false; + return nullptr; // Decide if we can write to the files.. string CacheFile = _config->FindFile("Dir::Cache::pkgcache"); @@ -820,15 +827,20 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, Writeable = access(flNotFile(SrcCacheFile).c_str(),W_OK) == 0; if (Writeable == false && AllowMem == false && CacheFile.empty() == false) - return _error->Error(_("Unable to write to %s"),flNotFile(CacheFile).c_str()); + { + _error->Error(_("Unable to write to %s"),flNotFile(CacheFile).c_str()); + return nullptr; + } Progress.OverallProgress(0,1,1,_("Reading Package Lists")); - // Cache is OK, Fin. - if (CheckValidity(CacheFile,Files.begin(),Files.end(),OutMap) == true) { - Progress.OverallProgress(1,1,1,_("Reading Package Lists")); - return true; + std::unique_ptr<MMap> Map = CheckValidity(CacheFile,Files.begin(),Files.end()); + if (Map) // Cache is OK, Fin. + { + Progress.OverallProgress(1,1,1,_("Reading Package Lists")); + return Map; + } } // CNC:2002-07-03 @@ -836,7 +848,7 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, if (_system->PreProcess(Files.begin(),Files.end(),Progress) == false) { _error->Error(_("Error pre-processing package lists")); - return false; + return nullptr; } #endif /* At this point we know we need to reconstruct the package cache, @@ -848,7 +860,7 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, unlink(CacheFile.c_str()); CacheF.reset(new FileFd(CacheFile,FileFd::WriteEmpty)); if (_error->PendingError() == true) - return false; + return nullptr; fchmod(CacheF->Fd(),0644); Map.reset(new DynamicMMap(*CacheF,MMap::Public,MapSize)); } @@ -862,13 +874,13 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, unsigned long CurrentSize = 0; unsigned long TotalSize = 0; if (CheckValidity(SrcCacheFile,Files.begin(), - Files.begin()+EndOfSource) == true) + Files.begin()+EndOfSource)) { // Preload the map with the source cache FileFd SCacheF(SrcCacheFile,FileFd::ReadOnly); if (SCacheF.Read((unsigned char *)Map->Data() + Map->RawAllocate(SCacheF.Size()), SCacheF.Size()) == false) - return false; + return nullptr; TotalSize = ComputeSize(Files.begin()+EndOfSource,Files.end()); @@ -881,10 +893,10 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, // Build the status cache pkgCacheGenerator Gen(*Map.get(),&Progress); if (_error->PendingError() == true) - return false; + return nullptr; if (BuildCache(Gen,Progress,CurrentSize,TotalSize, Files.begin()+EndOfSource,Files.end()) == false) - return false; + return nullptr; // CNC:2003-03-18 if (Gen.HasFileDeps() == true) { @@ -892,14 +904,14 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, Gen.GetCache().HeaderP->HasFileDeps = true; if (CollectFileProvides(Gen,Progress,CurrentSize,TotalSize, Files.begin(),Files.end()) == false) - return false; + return nullptr; } else if (Gen.GetCache().HeaderP->HasFileDeps == true) { // Jump entries which are not going to be parsed. CurrentSize += SrcSize; // No new file dependencies. Collect over the new packages. if (CollectFileProvides(Gen,Progress,CurrentSize,TotalSize, Files.begin()+EndOfSource,Files.end()) == false) - return false; + return nullptr; } } else @@ -915,10 +927,10 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, // Build the source cache pkgCacheGenerator Gen(*Map.get(),&Progress); if (_error->PendingError() == true) - return false; + return nullptr; if (BuildCache(Gen,Progress,CurrentSize,TotalSize, Files.begin(),Files.begin()+EndOfSource) == false) - return false; + return nullptr; // CNC:2003-11-24 Gen.GetCache().HeaderP->OptionsHash = _system->OptionsHash(); @@ -929,7 +941,7 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, Gen.GetCache().HeaderP->HasFileDeps = true; if (CollectFileProvides(Gen,Progress,CurrentSize,TotalSize, Files.begin(),Files.begin()+EndOfSource) == false) - return false; + return nullptr; // Reset to check for new file dependencies in the status cache. Gen.ResetFileDeps(); } else { @@ -947,19 +959,25 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, unlink(SrcCacheFile.c_str()); FileFd SCacheF(SrcCacheFile,FileFd::WriteEmpty); if (_error->PendingError() == true) - return false; + return nullptr; fchmod(SCacheF.Fd(),0644); // Write out the main data if (SCacheF.Write(Map->Data(),Map->Size()) == false) - return _error->Error(_("IO Error saving source cache")); + { + _error->Error(_("IO Error saving source cache")); + return nullptr; + } SCacheF.Sync(); // Write out the proper header Gen.GetCache().HeaderP->Dirty = false; if (SCacheF.Seek(0) == false || SCacheF.Write(Map->Data(),sizeof(*Gen.GetCache().HeaderP)) == false) - return _error->Error(_("IO Error saving source cache")); + { + _error->Error(_("IO Error saving source cache")); + return nullptr; + } Gen.GetCache().HeaderP->Dirty = true; SCacheF.Sync(); } @@ -967,7 +985,7 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, // Build the status cache if (BuildCache(Gen,Progress,CurrentSize,TotalSize, Files.begin()+EndOfSource,Files.end()) == false) - return false; + return nullptr; // CNC:2003-03-18 if (Gen.HasFileDeps() == true) { @@ -975,45 +993,39 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, Gen.GetCache().HeaderP->HasFileDeps = true; if (CollectFileProvides(Gen,Progress,CurrentSize,TotalSize, Files.begin(),Files.end()) == false) - return false; + return nullptr; } else if (Gen.GetCache().HeaderP->HasFileDeps == true) { // Jump entries which are not going to be parsed. CurrentSize += SrcSize; // No new file dependencies. Collect over the new packages. if (CollectFileProvides(Gen,Progress,CurrentSize,TotalSize, Files.begin()+EndOfSource,Files.end()) == false) - return false; + return nullptr; } } if (_error->PendingError() == true) - return false; - if (OutMap != 0) + return nullptr; + + if (CacheF != nullptr) { - if (CacheF != nullptr) - { - Map.reset(); - *OutMap = new MMap(*CacheF,MMap::Public | MMap::ReadOnly); - } - else - { - *OutMap = Map.release(); - } + Map.reset(); // don't delay freeing unneeded resources before making new MMap + return std::unique_ptr<MMap>(new MMap(*CacheF,MMap::Public | MMap::ReadOnly)); } - return true; + return Map; } /*}}}*/ // MakeOnlyStatusCache - Build a cache with just the status files /*{{{*/ // --------------------------------------------------------------------- /* */ -bool pkgMakeOnlyStatusCache(OpProgress &Progress,DynamicMMap **OutMap) +std::unique_ptr<DynamicMMap> pkgMakeOnlyStatusCache(OpProgress &Progress) { unsigned long MapSize = _config->FindI("APT::Cache-Limit",6*(16+sizeof(long))*1024*1024); vector<pkgIndexFile *> Files; unsigned long EndOfSource = Files.size(); if (_system->AddStatusFiles(Files) == false) - return false; + return nullptr; SPtr<DynamicMMap> Map(new DynamicMMap(MMap::Public,MapSize)); unsigned long CurrentSize = 0; @@ -1029,24 +1041,22 @@ bool pkgMakeOnlyStatusCache(OpProgress &Progress,DynamicMMap **OutMap) Progress.OverallProgress(0,1,1,_("Reading Package Lists")); pkgCacheGenerator Gen(*Map.get(),&Progress); if (_error->PendingError() == true) - return false; + return nullptr; if (BuildCache(Gen,Progress,CurrentSize,TotalSize, Files.begin()+EndOfSource,Files.end()) == false) - return false; + return nullptr; // CNC:2003-03-18 if (Gen.HasFileDeps() == true) { if (CollectFileProvides(Gen,Progress,CurrentSize,TotalSize, Files.begin()+EndOfSource,Files.end()) == false) - return false; + return nullptr; } if (_error->PendingError() == true) - return false; - *OutMap = Map.release(); - + return nullptr; - return true; + return Map; } /*}}}*/ diff --git a/apt-pkg/pkgcachegen.h b/apt-pkg/pkgcachegen.h index 0dea207b4..53847f8d5 100644 --- a/apt-pkg/pkgcachegen.h +++ b/apt-pkg/pkgcachegen.h @@ -19,6 +19,7 @@ #define PKGLIB_PKGCACHEGEN_H #include <apt-pkg/pkgcache.h> +#include <memory> class pkgSourceList; class OpProgress; @@ -139,20 +140,17 @@ class pkgCacheGenerator::ListParser virtual ~ListParser() {} }; -bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, - MMap **OutMap = 0,bool AllowMem = false); -bool pkgMakeOnlyStatusCache(OpProgress &Progress,DynamicMMap **OutMap); +std::unique_ptr<MMap> pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, + bool AllowMem = false); +std::unique_ptr<DynamicMMap> pkgMakeOnlyStatusCache(OpProgress &Progress); #ifdef APT_COMPATIBILITY #if APT_COMPATIBILITY != 986 #warning "Using APT_COMPATIBILITY" #endif -MMap *pkgMakeStatusCacheMem(pkgSourceList &List,OpProgress &Progress) +std::unique_ptr<MMap> pkgMakeStatusCacheMem(pkgSourceList &List,OpProgress &Progress) { - MMap *Map = 0; - if (pkgMakeStatusCache(List,Progress,&Map,true) == false) - return 0; - return Map; + return pkgMakeStatusCache(List,Progress,true); } #endif diff --git a/cmdline/apt-cache.cc b/cmdline/apt-cache.cc index 0c54ca82f..7dd9c9d00 100644 --- a/cmdline/apt-cache.cc +++ b/cmdline/apt-cache.cc @@ -1864,7 +1864,7 @@ bool GenCaches(CommandLine &Cmd) pkgSourceList List; if (List.ReadMainList() == false) return false; - return pkgMakeStatusCache(List,Progress); + return (pkgMakeStatusCache(List,Progress) != nullptr); } /*}}}*/ // ShowHelp - Show a help screen /*{{{*/ @@ -2016,11 +2016,11 @@ int main(int argc,const char *argv[]) if (CmdL.DispatchArg(CmdsA,false) == false && _error->PendingError() == false) { - MMap *Map = 0; + std::unique_ptr<MMap> Map; if (_config->FindB("APT::Cache::Generate",true) == false) { - Map = new MMap(*new FileFd(_config->FindFile("Dir::Cache::pkgcache"), - FileFd::ReadOnly),MMap::Public|MMap::ReadOnly); + Map.reset(new MMap(*new FileFd(_config->FindFile("Dir::Cache::pkgcache"), + FileFd::ReadOnly),MMap::Public|MMap::ReadOnly)); } else { @@ -2030,7 +2030,7 @@ int main(int argc,const char *argv[]) // Generate it and map it OpProgress Prog; - pkgMakeStatusCache(*SrcList,Prog,&Map,true); + Map = pkgMakeStatusCache(*SrcList,Prog,true); } if (_error->PendingError() == false) @@ -2057,7 +2057,6 @@ int main(int argc,const char *argv[]) if (_error->PendingError() == false) CmdL.DispatchArg(CmdsB); } - delete Map; } // Print any errors or warnings found during parsing
