From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 29 Jan 2020 19:24:33 +0300 (MSK) From: Ivan Zakharyaschev To: Aleksei Nikiforov In-Reply-To: <45228f59-3529-a3ee-7eb7-67eac012ffda@altlinux.org> Message-ID: References: <20200129012150.83E7E8440710@gitery.altlinux.org> <45228f59-3529-a3ee-7eb7-67eac012ffda@altlinux.org> User-Agent: Alpine 2.20 (LFD 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="1807885841-774305638-1580315073=:6363" Cc: Anton Farygin , devel@lists.altlinux.org, "Vladimir D. Seleznev" Subject: Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jan 2020 16:24:34 -0000 Archived-At: List-Archive: List-Post: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --1807885841-774305638-1580315073=:6363 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Hello! On Wed, 29 Jan 2020, Aleksei Nikiforov wrote: > Увидел данное изменение и по нему у меня появилось несколько вопросов. Хорошо, что обратил внимание. > Насколько я вижу, по большей части это rebase/cherry-pick одного из моих > коммитов, но с дополнительными изменениями поверх них. Почему не написать в > таком случае, что это изменение, сделанное Вами, и опционально указать на чём > оно основано, если считаете это нужным? Особенно если планируете использовать С этим нет проблем. Я могу оформить сообщение как кому хочется. > его в том виде, в котором он существует. Я спрашиваю этот вопрос из-за того, > что к дополнительным изменениям, сделанным помимо rebase и разрешения > возникших конфликтов, у меня есть вопросы. Хорошо, что обратил внимание и написал. Спасибо! Я хотел обсудить это и хотел, как мы это завели, написать в devel@ свои вопросы и просьбу посмотреть, прокомментировать. Сейчас как раз это и случится, раз обсуждение уже пошло. > Помимо вопроса где планируется использовать данное изменение, поскольку в Я просто уделяю внимание тем изменениям, которые были в последних релизах, и разбираюсь в них, навожу порядок в понимании и оформлении, чтобы у читающего было ясное понимание, почему эти изменения происходят. (Чиатющим могу быть я в будущем или кто-то другой.) Прежде всего, отвлекаясь от вопросов, скажу, почему я его перенёс в истории пораньше: Обсуждение арифметики указателей ещё не закончилось (и прочее, связанное с динамическим realloc/mremap, сложнее понять), а это простое по сути изменение, которые и читаемость и понимание кода улучшает на мой взгляд, поэтому я его переставил в истории пораньше. (Как более бесспорно правильное и как облегчающее понимание кода и логики APT-а, которое поможет понимать дальнейшие изменения.) Наведение порядка в истории и в голове -- это значит для меня, что изменение делается по одной причине (тут -- переход на optional) и не смешивается с другими. Другие изменение должны получить отдельную запись о том, почему они оправданы. > Сизифе и p9 оно уже присутствует, в первую очередь у меня вопрос к следующему > куску кода: > > > diff --git a/apt/apt-pkg/rpm/rpmindexfile.cc > b/apt/apt-pkg/rpm/rpmindexfile.cc > > index 271cd8f..9a8a7f9 100644 > > --- a/apt/apt-pkg/rpm/rpmindexfile.cc > > +++ b/apt/apt-pkg/rpm/rpmindexfile.cc > > @@ -406,8 +406,10 @@ bool rpmPkgListIndex::Merge(pkgCacheGenerator > &Gen,OpProgress &Prog) const > > FileFd Rel(RelFile,FileFd::ReadOnly); > > if (_error->PendingError() == true) > > return false; > > - Parser.LoadReleaseInfo(File,Rel); > > + const bool Result = Parser.LoadReleaseInfo(File,Rel); > > Rel.Seek(0); > > + if (!Result) > > + return false; > > } > > > > return true; > > По сравнению с моей версией, тут не убран вызов "Rel.Seek(0)". Мне не понятна > причина такого изменения по сравнению с кодом, на котором основано данное > изменение. Да, конечно, я это заметил. Но Rel.Seek(0) никак не связан с введением optional в API и я совершенно не знал, когда это увидел, почему он был убран. Объяснений, которые я мог бы прочитать, не нашёл. Если это оправданное изменение, будет хорошо такой коммит с ним будет сделан отдельно с объяснением. Спасибо за сообщение, сейчас почитаю (наверное, именно это я и хотел узнать): > Если посмотреть внимательно, "FileFd Rel" объявлен несколькими > строками выше, и его область видимости вскоре после этой строки заканчивается, > а следовательно данный объект никак не используется после вызова > Parser.LoadReleaseInfo(...) и вскоре уничтожается. Данный вызов Rel.Seek(...) > - небольшая обёртка над lseek. Делать lseek перед закрытием файла смысла не > имеет, соответственно нет смысла и оставлять этот мёртвый код, вызов > Rel.Seek(...), или я что-то пропустил? А есть идеи, почему он там раньше был? Была бессмысленная операция? (В любом случае, я рад любым улучшениям кода, но хотел бы очень попросить во всех коммитах в будущем не смешивать изменения, которые объясняются разными целями.) > Вот второй фрагмент изменений под вопросом: > > > @@ -544,18 +562,24 @@ bool > pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver, > > #endif > > > > // Get a structure > > - unsigned long Provides = > Owner->AllocateInMap(sizeof(pkgCache::Provides)); > > - if (Provides == 0) > > + const auto Provides = > Owner->AllocateInMap(sizeof(pkgCache::Provides)); > > + const auto idxVersion = Version.empty() > > + ? std::experimental::optional() > > + : WriteString(Version); > > + if (!Provides || (!Version.empty() && !idxVersion)) > > return false; > > Cache.HeaderP->ProvidesCount++; > > > > // Fill it in > > - pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + > Provides,Cache.PkgP); > > + pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + > *Provides,Cache.PkgP); > > Prv->Version = Ver.Index(); > > Prv->NextPkgProv = Ver->ProvidesList; > > Ver->ProvidesList = Prv.Index(); > > - if (Version.empty() == false && (Prv->ProvideVersion = > WriteString(Version)) == 0) > > - return false; > > + > > + if (Version.empty() == false) > > + { > > + Prv->ProvideVersion = *idxVersion; > > + } > > > > // Locate the target package > > pkgCache::PkgIterator Pkg; > > Здесь мне следующее условие кажется неоправданно усложнённым и трудночитаемым > по сравнение с кодом из моего изменения: > > if (!Provides || (!Version.empty() && !idxVersion)) > > Конечно, за счёт этого в условии "if (Version.empty() == false)" код сокращён > заметно по сравнению с моей версией, но все те строки, хоть их было и больше, > были простыми. Замена их на несколько усложнённых строк - не то, что я выбрал > бы в данном случае. Или в данном случае я тоже пропустил какую-то причину > данного изменения? Не то, чтобы есть практические причины, но вызывало удивление и вопрос в твоей версии, что заполняется бессмысленное значение 0 в случае, если довести эту "транзакцию" по выделению и заполнению памяти не удалось довести успешно до конца: // Get a structure const auto Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides)); - if (Provides == 0) + if (!Provides) return false; Cache.HeaderP->ProvidesCount++; // Fill it in - pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + Provides,Cache.PkgP); + pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + *Provides,Cache.PkgP); Dynamic DynPrv(Prv); Prv->Version = Ver.Index(); Prv->NextPkgProv = Ver->ProvidesList; Ver->ProvidesList = Prv.Index(); - if (Version.empty() == false && (Prv->ProvideVersion = WriteString(Version)) == 0) - return false; + + if (Version.empty() == false) + { + const auto idxVersion = WriteString(Version); + if (!idxVersion) + { + Prv->ProvideVersion = 0; + return false; + } + + Prv->ProvideVersion = *idxVersion; + } Чтобы не было удивления от бессмысленного значения, я переписал так, как ты увидел. Не люблю операции, которые какой-то мусор присваивают, просто чтобы место чем-то заполнить. В моём варианте Prv даже не конструируется и не начинает заполняться, если не удалось выделить память в Map под строку Version. Это же понятнее и лишних вопросов не вызвает: что потом с этой частично осмысленно заполненной, частично незаполненной структурой произойдёт и кто её в таком виде увидит дальше. > 29.01.2020 4:21, Ivan Zakharyaschev пишет: > > Update of /people/imz/packages/apt.git > > > > Changes statistics since common ancestor `0.5.15lorg2-alt68.1-12-g297a12d' > > follows: > > apt.spec | 2 +- > > apt/apt-pkg/contrib/mmap.cc | 36 ++++++--- > > apt/apt-pkg/contrib/mmap.h | 9 ++- > > apt/apt-pkg/pkgcachegen.cc | 159 > > +++++++++++++++++++++++---------------- > > apt/apt-pkg/pkgcachegen.h | 24 +++--- > > apt/apt-pkg/rpm/rpmindexfile.cc | 4 +- > > apt/apt-pkg/rpm/rpmlistparser.cc | 78 ++++++++++++++++--- > > apt/apt-pkg/rpm/rpmlistparser.h | 3 +- > > 8 files changed, 207 insertions(+), 108 deletions(-) > > > > Changelog since common ancestor `0.5.15lorg2-alt68.1-12-g297a12d' follows: > > commit 6fc29243356a0ef1f3a93749114f3f0fc95992af > > Author: Aleksei Nikiforov > > Date: Thu Jun 13 12:35:49 2019 +0300 > > > > Use special type to return allocation failure since 0 is a valid offset > > value > > > > (cherry picked from commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6) > > > > Rebased and improved by: > > Ivan Zakharyaschev and > > Vladimir D. Seleznev . > > > > Full diff since common ancestor `0.5.15lorg2-alt68.1-12-g297a12d' follows: > > diff --git a/apt.spec b/apt.spec > > index 362179e..4b18a94 100644 > > --- a/apt.spec > > +++ b/apt.spec > > @@ -230,7 +230,7 @@ printf '%_target_cpu\t%_target_cpu' >> > > buildlib/archtable > > %autoreconf > > %add_optflags > > %-DAPTRPM_ID=\\\"%name-%{?epoch:%epoch:}%version-%release%{?disttag::%disttag}.%_target_cpu\\\" > > %ifarch %e2k > > -%add_optflags -std=gnu++11 > > +%add_optflags -std=c++14 > > %endif > > %configure --includedir=%_includedir/apt-pkg %{subst_enable static} > > diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc > > index ef9e312..781bed0 100644 > > --- a/apt/apt-pkg/contrib/mmap.cc > > +++ b/apt/apt-pkg/contrib/mmap.cc > > @@ -202,7 +202,7 @@ DynamicMMap::~DynamicMMap() > > // DynamicMMap::RawAllocate - Allocate a raw chunk of unaligned space > > /*{{{*/ > > // --------------------------------------------------------------------- > > /* This allocates a block of memory aligned to the given size */ > > -unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long > > Aln) > > +std::experimental::optional > > DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) > > { > > unsigned long long Result = iSize; > > if (Aln != 0) > > @@ -214,18 +214,24 @@ unsigned long DynamicMMap::RawAllocate(unsigned long > > Size,unsigned long Aln) > > if (Result + Size > WorkSpace) > > { > > _error->Error("Dynamic MMap ran out of room"); > > - return 0; > > + return std::experimental::optional(); > > } > > - return Result; > > + return std::experimental::optional(Result); > > } > > /*}}}*/ > > // DynamicMMap::Allocate - Pooled aligned allocation > > /*{{{*/ > > // --------------------------------------------------------------------- > > /* This allocates an Item of size ItemSize so that it is aligned to its > > size in the file. */ > > -unsigned long DynamicMMap::Allocate(unsigned long ItemSize) > > -{ > > +std::experimental::optional DynamicMMap::Allocate(unsigned > > long ItemSize) > > +{ > > + if (ItemSize == 0) > > + { > > + _error->Error("Can't allocate an item of size zero"); > > + return std::experimental::optional(); > > + } > > + > > // Look for a matching pool entry > > Pool *I; > > Pool *Empty = 0; > > @@ -244,7 +250,7 @@ unsigned long DynamicMMap::Allocate(unsigned long > > ItemSize) > > if (Empty == 0) > > { > > _error->Error("Ran out of allocation pools"); > > - return 0; > > + return std::experimental::optional(); > > } > > > > I = Empty; > > @@ -256,19 +262,25 @@ unsigned long DynamicMMap::Allocate(unsigned long > > ItemSize) > > if (I->Count == 0) > > { > > I->Count = 20*1024/ItemSize; > > - I->Start = RawAllocate(I->Count*ItemSize,ItemSize); > > - } > > + auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize); > > + > > + // Does the allocation failed ? > > + if (!idxResult) > > + return idxResult; > > + > > + I->Start = *idxResult; > > + } > > > > I->Count--; > > unsigned long Result = I->Start; > > I->Start += ItemSize; > > - return Result/ItemSize; > > + return std::experimental::optional(Result/ItemSize); > > } > > /*}}}*/ > > // DynamicMMap::WriteString - Write a string to the file > > /*{{{*/ > > // --------------------------------------------------------------------- > > /* Strings are not aligned to anything */ > > -unsigned long DynamicMMap::WriteString(const char *String, > > +std::experimental::optional DynamicMMap::WriteString(const > > char *String, > > unsigned long Len) > > { > > unsigned long Result = iSize; > > @@ -276,7 +288,7 @@ unsigned long DynamicMMap::WriteString(const char > > *String, > > if (Result + Len > WorkSpace) > > { > > _error->Error("Dynamic MMap ran out of room"); > > - return 0; > > + return std::experimental::optional(); > > } > > > > if (Len == (unsigned long)-1) > > @@ -284,6 +296,6 @@ unsigned long DynamicMMap::WriteString(const char > > *String, > > iSize += Len + 1; > > memcpy((char *)Base + Result,String,Len); > > ((char *)Base)[Result + Len] = 0; > > - return Result; > > + return std::experimental::optional(Result); > > } > > /*}}}*/ > > diff --git a/apt/apt-pkg/contrib/mmap.h b/apt/apt-pkg/contrib/mmap.h > > index efc5245..14df055 100644 > > --- a/apt/apt-pkg/contrib/mmap.h > > +++ b/apt/apt-pkg/contrib/mmap.h > > @@ -29,6 +29,7 @@ > > #endif > > > > #include > > +#include > > #include > > > > using std::string; > > @@ -90,10 +91,10 @@ class DynamicMMap : public MMap > > public: > > > > // Allocation > > - unsigned long RawAllocate(unsigned long Size,unsigned long Aln = 0); > > - unsigned long Allocate(unsigned long ItemSize); > > - unsigned long WriteString(const char *String,unsigned long Len = > > (unsigned long)-1); > > - inline unsigned long WriteString(const string &S) {return > > WriteString(S.c_str(),S.length());}; > > + std::experimental::optional RawAllocate(unsigned long > > Size,unsigned long Aln = 0); > > + std::experimental::optional Allocate(unsigned long > > ItemSize); > > + std::experimental::optional WriteString(const char > > *String,unsigned long Len = (unsigned long)-1); > > + inline std::experimental::optional WriteString(const > > string &S) {return WriteString(S.c_str(),S.length());}; > > void UsePools(Pool &P,unsigned int Count) {Pools = &P; PoolCount = > > Count;}; > > > > DynamicMMap(FileFd &F,unsigned long Flags,unsigned long WorkSpace = > > 2*1024*1024); > > diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc > > index 070c0f0..ae0d4e3 100644 > > --- a/apt/apt-pkg/pkgcachegen.cc > > +++ b/apt/apt-pkg/pkgcachegen.cc > > @@ -54,16 +54,22 @@ pkgCacheGenerator::pkgCacheGenerator(DynamicMMap > > *pMap,OpProgress *Prog) : > > > > if (Map.Size() == 0) > > { > > + const auto idxAlloc = Map.RawAllocate(sizeof(pkgCache::Header)); > > + const auto idxVerSysName = WriteStringInMap(_system->VS->Label); > > + const auto idxArch = > > WriteStringInMap(_config->Find("APT::Architecture")); > > + if ((!idxAlloc) || (!idxVerSysName) || (!idxArch)) > > + return; > > + > > // Setup the map interface.. > > - Cache.HeaderP = (pkgCache::Header *)Map.Data(); > > - Map.RawAllocate(sizeof(pkgCache::Header)); > > + Cache.HeaderP = (pkgCache::Header *)(static_cast(Map.Data()) + > > *idxAlloc); > > + > > Map.UsePools(*Cache.HeaderP->Pools,sizeof(Cache.HeaderP->Pools)/sizeof(Cache.HeaderP->Pools[0])); > > > > // Starting header > > *Cache.HeaderP = pkgCache::Header(); > > - Cache.HeaderP->VerSysName = WriteStringInMap(_system->VS->Label); > > - Cache.HeaderP->Architecture = > > WriteStringInMap(_config->Find("APT::Architecture")); > > - Cache.ReMap(); > > + Cache.HeaderP->VerSysName = *idxVerSysName; > > + Cache.HeaderP->Architecture = *idxArch; > > + Cache.ReMap(); > > } > > else > > { > > @@ -97,17 +103,17 @@ pkgCacheGenerator::~pkgCacheGenerator() > > } > > /*}}}*/ > > // CacheGenerator::WriteStringInMap > > /*{{{*/ > > -unsigned long pkgCacheGenerator::WriteStringInMap(const char *String, > > +std::experimental::optional > > pkgCacheGenerator::WriteStringInMap(const char *String, > > unsigned long Len) { > > return Map.WriteString(String, Len); > > } > > /*}}}*/ > > // CacheGenerator::WriteStringInMap > > /*{{{*/ > > -unsigned long pkgCacheGenerator::WriteStringInMap(const char *String) { > > +std::experimental::optional > > pkgCacheGenerator::WriteStringInMap(const char *String) { > > return Map.WriteString(String); > > } > > /*}}}*/ > > -unsigned long pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/ > > +std::experimental::optional > > pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/ > > return Map.Allocate(size); > > } > > /*}}}*/ > > @@ -221,7 +227,12 @@ bool pkgCacheGenerator::MergeList(ListParser &List, > > } > > > > // Add a new version > > - *Last = NewVersion(Ver,Version,*Last); > > + const auto verindex = NewVersion(Ver,Version,*Last); > > + if (!verindex) > > + return _error->Error(_("Error occurred while processing %s > > (NewVersion0)"), > > + PackageName.c_str()); > > + *Last = *verindex; > > + > > Ver->ParentPkg = Pkg.Index(); > > Ver->Hash = Hash; > > if (List.NewVersion(Ver) == false) > > @@ -365,21 +376,20 @@ bool > > pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, const string &Nam > > #endif > > > > // Get a structure > > - unsigned long Package = AllocateInMap(sizeof(pkgCache::Package)); > > - if (Package == 0) > > + const auto Package = AllocateInMap(sizeof(pkgCache::Package)); > > + const auto idxName = WriteStringInMap(Name); > > + if ((!Package) || (!idxName)) > > return false; > > - > > - Pkg = pkgCache::PkgIterator(Cache,Cache.PkgP + Package); > > - > > + > > + Pkg = pkgCache::PkgIterator(Cache,Cache.PkgP + *Package); > > + > > // Insert it into the hash table > > unsigned long Hash = Cache.Hash(Name); > > Pkg->NextPackage = Cache.HeaderP->HashTable[Hash]; > > - Cache.HeaderP->HashTable[Hash] = Package; > > - > > + Cache.HeaderP->HashTable[Hash] = *Package; > > + > > // Set the name and the ID > > - Pkg->Name = WriteStringInMap(Name); > > - if (Pkg->Name == 0) > > - return false; > > + Pkg->Name = *idxName; > > Pkg->ID = Cache.HeaderP->PackageCount++; > > > > return true; > > @@ -407,7 +417,11 @@ bool > > pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, > > if ( (':' - '@' < 0 ? rc < 0 : rc > 0) || > > (rc != 0 && List.IsDatabase()) ) > > { > > - Ver->VerStr = WriteStringInMap(Version); > > + const auto verStrIdx = WriteStringInMap(Version); > > + if (!verStrIdx) > > + return false; > > + > > + Ver->VerStr = *verStrIdx; > > } > > } > > @@ -415,11 +429,11 @@ bool > > pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, > > return true; > > > > // Get a structure > > - unsigned long VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); > > - if (VerFile == 0) > > - return 0; > > + const auto VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); > > + if (!VerFile) > > + return false; > > - pkgCache::VerFileIterator VF(Cache,Cache.VerFileP + VerFile); > > + pkgCache::VerFileIterator VF(Cache,Cache.VerFileP + *VerFile); > > VF->File = CurrentFile - Cache.PkgFileP; > > > > // Link it to the end of the list > > @@ -441,23 +455,22 @@ bool > > pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, > > // CacheGenerator::NewVersion - Create a new Version > > /*{{{*/ > > // --------------------------------------------------------------------- > > /* This puts a version structure in the linked list */ > > -unsigned long pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, > > +std::experimental::optional > > pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, > > const string &VerStr, > > unsigned long Next) > > { > > // Get a structure > > - unsigned long Version = AllocateInMap(sizeof(pkgCache::Version)); > > - if (Version == 0) > > - return 0; > > - > > + const auto Version = AllocateInMap(sizeof(pkgCache::Version)); > > + const auto idxVerStr = WriteStringInMap(VerStr); > > + if ((!Version) || (!idxVerStr)) > > + return std::experimental::optional(); > > + > > // Fill it in > > - Ver = pkgCache::VerIterator(Cache,Cache.VerP + Version); > > + Ver = pkgCache::VerIterator(Cache,Cache.VerP + *Version); > > Ver->NextVer = Next; > > Ver->ID = Cache.HeaderP->VersionCount++; > > - Ver->VerStr = WriteStringInMap(VerStr); > > - if (Ver->VerStr == 0) > > - return 0; > > - > > + Ver->VerStr = *idxVerStr; > > + > > return Version; > > } > > /*}}}*/ > > @@ -474,12 +487,12 @@ bool > > pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver, > > pkgCache &Cache = Owner->Cache; > > > > // Get a structure > > - unsigned long Dependency = > > Owner->AllocateInMap(sizeof(pkgCache::Dependency)); > > - if (Dependency == 0) > > + const auto Dependency = > > Owner->AllocateInMap(sizeof(pkgCache::Dependency)); > > + if (!Dependency) > > return false; > > > > // Fill it in > > - pkgCache::DepIterator Dep(Cache,Cache.DepP + Dependency); > > + pkgCache::DepIterator Dep(Cache,Cache.DepP + *Dependency); > > Dep->ParentVer = Ver.Index(); > > Dep->Type = Type; > > Dep->CompareOp = Op; > > @@ -497,8 +510,13 @@ bool > > pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver, > > if (I->Version != 0 && I.TargetVer() == Version) > > Dep->Version = I->Version;*/ > > if (Dep->Version == 0) > > - if ((Dep->Version = WriteString(Version)) == 0) > > - return false; > > + { > > + const auto index = WriteString(Version); > > + if (!index) > > + return false; > > + > > + Dep->Version = *index; > > + } > > } > > > > // Link it to the package > > @@ -544,18 +562,24 @@ bool > > pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver, > > #endif > > > > // Get a structure > > - unsigned long Provides = > > Owner->AllocateInMap(sizeof(pkgCache::Provides)); > > - if (Provides == 0) > > + const auto Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides)); > > + const auto idxVersion = Version.empty() > > + ? std::experimental::optional() > > + : WriteString(Version); > > + if (!Provides || (!Version.empty() && !idxVersion)) > > return false; > > Cache.HeaderP->ProvidesCount++; > > > > // Fill it in > > - pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + Provides,Cache.PkgP); > > + pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + *Provides,Cache.PkgP); > > Prv->Version = Ver.Index(); > > Prv->NextPkgProv = Ver->ProvidesList; > > Ver->ProvidesList = Prv.Index(); > > - if (Version.empty() == false && (Prv->ProvideVersion = > > WriteString(Version)) == 0) > > - return false; > > + > > + if (Version.empty() == false) > > + { > > + Prv->ProvideVersion = *idxVersion; > > + } > > > > // Locate the target package > > pkgCache::PkgIterator Pkg; > > @@ -579,23 +603,24 @@ bool pkgCacheGenerator::SelectFile(const string &File, > > const string &Site, > > unsigned long Flags) > > { > > // Get some space for the structure > > - CurrentFile = Cache.PkgFileP + AllocateInMap(sizeof(*CurrentFile)); > > - if (CurrentFile == Cache.PkgFileP) > > + const auto idxFile = AllocateInMap(sizeof(*CurrentFile)); > > + const auto idxFileName = WriteStringInMap(File); > > + const auto idxSite = WriteUniqString(Site); > > + const auto idxIndexType = WriteUniqString(Index.GetType()->Label); > > + if ((!idxFile) || (!idxFileName) || (!idxSite) || (!idxIndexType)) > > return false; > > + CurrentFile = Cache.PkgFileP + *idxFile; > > > > // Fill it in > > - CurrentFile->FileName = WriteStringInMap(File); > > - CurrentFile->Site = WriteUniqString(Site); > > + CurrentFile->FileName = *idxFileName; > > + CurrentFile->Site = *idxSite; > > CurrentFile->NextFile = Cache.HeaderP->FileList; > > CurrentFile->Flags = Flags; > > CurrentFile->ID = Cache.HeaderP->PackageFileCount; > > - CurrentFile->IndexType = WriteUniqString(Index.GetType()->Label); > > + CurrentFile->IndexType = *idxIndexType; > > PkgFileName = File; > > Cache.HeaderP->FileList = CurrentFile - Cache.PkgFileP; > > Cache.HeaderP->PackageFileCount++; > > - > > - if (CurrentFile->FileName == 0) > > - return false; > > > > if (Progress != 0) > > Progress->SubProgress(Index.Size()); > > @@ -606,7 +631,7 @@ bool pkgCacheGenerator::SelectFile(const string &File, > > const string &Site, > > // --------------------------------------------------------------------- > > /* This is used to create handles to strings. Given the same text it > > always returns the same number */ > > -unsigned long pkgCacheGenerator::WriteUniqString(const char *S, > > +std::experimental::optional > > pkgCacheGenerator::WriteUniqString(const char *S, > > unsigned int Size) > > { > > /* We use a very small transient hash table here, this speeds up > > generation > > @@ -614,7 +639,7 @@ unsigned long pkgCacheGenerator::WriteUniqString(const > > char *S, > > pkgCache::StringItem *&Bucket = UniqHash[(S[0]*5 + S[1]) % > > _count(UniqHash)]; > > if (Bucket != 0 && > > stringcmp(S,S+Size,Cache.StrP + Bucket->String) == 0) > > - return Bucket->String; > > + return std::experimental::optional(Bucket->String); > > > > // Search for an insertion point > > pkgCache::StringItem *I = Cache.StringItemP + > > Cache.HeaderP->StringList; > > @@ -632,24 +657,23 @@ unsigned long pkgCacheGenerator::WriteUniqString(const > > char *S, > > if (Res == 0) > > { > > Bucket = I; > > - return I->String; > > + return std::experimental::optional(I->String); > > } > > > > // Get a structure > > - unsigned long Item = AllocateInMap(sizeof(pkgCache::StringItem)); > > - if (Item == 0) > > - return 0; > > + const auto Item = AllocateInMap(sizeof(pkgCache::StringItem)); > > + const auto idxString = WriteStringInMap(S, Size); > > + if ((!Item) || (!idxString)) > > + return std::experimental::optional(); > > > > // Fill in the structure > > - pkgCache::StringItem *ItemP = Cache.StringItemP + Item; > > + pkgCache::StringItem *ItemP = Cache.StringItemP + *Item; > > ItemP->NextItem = I - Cache.StringItemP; > > - *Last = Item; > > - ItemP->String = WriteStringInMap(S,Size); > > - if (ItemP->String == 0) > > - return 0; > > - > > + *Last = *Item; > > + ItemP->String = *idxString; > > + > > Bucket = ItemP; > > - return ItemP->String; > > + return std::experimental::optional(ItemP->String); > > } > > /*}}}*/ > > @@ -882,7 +906,10 @@ bool pkgMakeStatusCache(pkgSourceList > > &List,OpProgress &Progress, > > { > > // Preload the map with the source cache > > FileFd SCacheF(SrcCacheFile,FileFd::ReadOnly); > > - if (SCacheF.Read((unsigned char *)Map->Data() + > > Map->RawAllocate(SCacheF.Size()), > > + const auto idxAllocate = Map->RawAllocate(SCacheF.Size()); > > + if (!idxAllocate) > > + return false; > > + if (SCacheF.Read((unsigned char *)Map->Data() + *idxAllocate, > > SCacheF.Size()) == false) > > return false; > > diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h > > index a187cd1..69c4f2e 100644 > > --- a/apt/apt-pkg/pkgcachegen.h > > +++ b/apt/apt-pkg/pkgcachegen.h > > @@ -24,6 +24,8 @@ > > > > #include > > > > +#include > > + > > class pkgSourceList; > > class OpProgress; > > class MMap; > > @@ -34,10 +36,10 @@ class pkgCacheGenerator > > private: > > > > pkgCache::StringItem *UniqHash[26]; > > - unsigned long WriteStringInMap(const std::string &String) { return > > WriteStringInMap(String.c_str(), String.length()); }; > > - unsigned long WriteStringInMap(const char *String); > > - unsigned long WriteStringInMap(const char *String, unsigned long Len); > > - unsigned long AllocateInMap(unsigned long size); > > + std::experimental::optional WriteStringInMap(const > > std::string &String) { return WriteStringInMap(String.c_str(), > > String.length()); }; > > + std::experimental::optional WriteStringInMap(const char > > *String); > > + std::experimental::optional WriteStringInMap(const char > > *String, unsigned long Len); > > + std::experimental::optional AllocateInMap(unsigned long > > size); > > > > public: > > > > @@ -57,15 +59,15 @@ class pkgCacheGenerator > > bool FoundFileDeps; > > > > bool NewFileVer(pkgCache::VerIterator &Ver,ListParser &List); > > - unsigned long NewVersion(pkgCache::VerIterator &Ver,const string > > &VerStr,unsigned long Next); > > + std::experimental::optional > > NewVersion(pkgCache::VerIterator &Ver,const string &VerStr,unsigned long > > Next); > > > > public: > > > > // CNC:2003-02-27 - We need this in rpmListParser. > > bool NewPackage(pkgCache::PkgIterator &PkgI,const string &Pkg); > > - unsigned long WriteUniqString(const char *S,unsigned int Size); > > - inline unsigned long WriteUniqString(const string &S) {return > > WriteUniqString(S.c_str(),S.length());}; > > + std::experimental::optional WriteUniqString(const char > > *S,unsigned int Size); > > + inline std::experimental::optional WriteUniqString(const > > string &S) {return WriteUniqString(S.c_str(),S.length());}; > > > > void DropProgress() {Progress = 0;}; > > bool SelectFile(const string &File,const string &Site,pkgIndexFile > > const &Index, > > @@ -101,10 +103,10 @@ class pkgCacheGenerator::ListParser > > pkgCacheGenerator *Owner; > > friend class pkgCacheGenerator; > > - inline unsigned long WriteUniqString(const string &S) {return > > Owner->WriteUniqString(S);}; > > - inline unsigned long WriteUniqString(const char *S,unsigned int Size) > > {return Owner->WriteUniqString(S,Size);}; > > - inline unsigned long WriteString(const string &S) {return > > Owner->WriteStringInMap(S);}; > > - inline unsigned long WriteString(const char *S,unsigned int Size) > > {return Owner->WriteStringInMap(S,Size);}; > > + inline std::experimental::optional WriteUniqString(const > > string &S) {return Owner->WriteUniqString(S);}; > > + inline std::experimental::optional WriteUniqString(const > > char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);}; > > + inline std::experimental::optional WriteString(const > > string &S) {return Owner->WriteStringInMap(S);}; > > + inline std::experimental::optional WriteString(const char > > *S,unsigned int Size) {return Owner->WriteStringInMap(S,Size);}; > > bool NewDepends(pkgCache::VerIterator &Ver, const string &Package, > > const string &Version,unsigned int Op, > > unsigned int Type); > > diff --git a/apt/apt-pkg/rpm/rpmindexfile.cc > > b/apt/apt-pkg/rpm/rpmindexfile.cc > > index 271cd8f..9a8a7f9 100644 > > --- a/apt/apt-pkg/rpm/rpmindexfile.cc > > +++ b/apt/apt-pkg/rpm/rpmindexfile.cc > > @@ -406,8 +406,10 @@ bool rpmPkgListIndex::Merge(pkgCacheGenerator > > &Gen,OpProgress &Prog) const > > FileFd Rel(RelFile,FileFd::ReadOnly); > > if (_error->PendingError() == true) > > return false; > > - Parser.LoadReleaseInfo(File,Rel); > > + const bool Result = Parser.LoadReleaseInfo(File,Rel); > > Rel.Seek(0); > > + if (!Result) > > + return false; > > } > > > > return true; > > diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc > > b/apt/apt-pkg/rpm/rpmlistparser.cc > > index 2fc4e01..7b946bf 100644 > > --- a/apt/apt-pkg/rpm/rpmlistparser.cc > > +++ b/apt/apt-pkg/rpm/rpmlistparser.cc > > @@ -70,7 +70,7 @@ rpmListParser::~rpmListParser() > > // ListParser::UniqFindTagWrite - Find the tag and write a unq string > > /*{{{*/ > > // --------------------------------------------------------------------- > > /* */ > > -unsigned long rpmListParser::UniqFindTagWrite(int Tag) > > +std::experimental::optional > > rpmListParser::UniqFindTagWrite(int Tag) > > { > > char *Start; > > char *Stop; > > @@ -79,7 +79,7 @@ unsigned long rpmListParser::UniqFindTagWrite(int Tag) > > void *data; > > > > if (headerGetEntry(header, Tag, &type, &data, &count) != 1) > > - return 0; > > + return std::experimental::optional(); > > > > if (type == RPM_STRING_TYPE) > > { > > @@ -255,8 +255,13 @@ bool rpmListParser::NewVersion(pkgCache::VerIterator > > &Ver) > > #endif > > > > // Parse the section > > - Ver->Section = UniqFindTagWrite(RPMTAG_GROUP); > > - Ver->Arch = UniqFindTagWrite(RPMTAG_ARCH); > > + const auto idxSection = UniqFindTagWrite(RPMTAG_GROUP); > > + const auto idxArch = UniqFindTagWrite(RPMTAG_ARCH); > > + if ((!idxSection) || (!idxArch)) > > + return false; > > + > > + Ver->Section = *idxSection; > > + Ver->Arch = *idxArch; > > > > // Archive Size > > Ver->Size = Handler->FileSize(); > > @@ -296,7 +301,14 @@ bool rpmListParser::UsePackage(pkgCache::PkgIterator > > &Pkg, > > if (SeenPackages != NULL) > > (*SeenPackages)[Pkg.Name()] = true; > > if (Pkg->Section == 0) > > - Pkg->Section = UniqFindTagWrite(RPMTAG_GROUP); > > + { > > + const auto idxSection = UniqFindTagWrite(RPMTAG_GROUP); > > + if (!idxSection) > > + return false; > > + > > + Pkg->Section = *idxSection; > > + } > > + > > if (_error->PendingError()) > > return false; > > string PkgName = Pkg.Name(); > > @@ -740,19 +752,61 @@ bool > > rpmListParser::LoadReleaseInfo(pkgCache::PkgFileIterator &FileI, > > > > const char *Start; > > const char *Stop; > > + > > if (Section.Find("Archive",Start,Stop)) > > - FileI->Archive = WriteUniqString(Start,Stop - Start); > > + { > > + const auto idx = WriteUniqString(Start, Stop - Start); > > + if (!idx) > > + return false; > > + > > + FileI->Archive = *idx; > > + } > > + > > if (Section.Find("Component",Start,Stop)) > > - FileI->Component = WriteUniqString(Start,Stop - Start); > > + { > > + const auto idx = WriteUniqString(Start, Stop - Start); > > + if (!idx) > > + return false; > > + > > + FileI->Component = *idx; > > + } > > + > > if (Section.Find("Version",Start,Stop)) > > - FileI->Version = WriteUniqString(Start,Stop - Start); > > + { > > + const auto idx = WriteUniqString(Start, Stop - Start); > > + if (!idx) > > + return false; > > + > > + FileI->Version = *idx; > > + } > > + > > if (Section.Find("Origin",Start,Stop)) > > - FileI->Origin = WriteUniqString(Start,Stop - Start); > > + { > > + const auto idx = WriteUniqString(Start, Stop - Start); > > + if (!idx) > > + return false; > > + > > + FileI->Origin = *idx; > > + } > > + > > if (Section.Find("Label",Start,Stop)) > > - FileI->Label = WriteUniqString(Start,Stop - Start); > > + { > > + const auto idx = WriteUniqString(Start, Stop - Start); > > + if (!idx) > > + return false; > > + > > + FileI->Label = *idx; > > + } > > + > > if (Section.Find("Architecture",Start,Stop)) > > - FileI->Architecture = WriteUniqString(Start,Stop - Start); > > - > > + { > > + const auto idx = WriteUniqString(Start, Stop - Start); > > + if (!idx) > > + return false; > > + > > + FileI->Architecture = *idx; > > + } > > + > > if (Section.FindFlag("NotAutomatic",FileI->Flags, > > pkgCache::Flag::NotAutomatic) == false) > > _error->Warning(_("Bad NotAutomatic flag")); > > diff --git a/apt/apt-pkg/rpm/rpmlistparser.h > > b/apt/apt-pkg/rpm/rpmlistparser.h > > index 5c6173c..dd57f7c 100644 > > --- a/apt/apt-pkg/rpm/rpmlistparser.h > > +++ b/apt/apt-pkg/rpm/rpmlistparser.h > > @@ -20,6 +20,7 @@ > > #include > > #include > > #include > > +#include > > > > using namespace std; > > > > @@ -45,7 +46,7 @@ class rpmListParser : public pkgCacheGenerator::ListParser > > > > bool Duplicated; > > > > - unsigned long UniqFindTagWrite(int Tag); > > + std::experimental::optional UniqFindTagWrite(int Tag); > > bool ParseStatus(pkgCache::PkgIterator &Pkg,pkgCache::VerIterator > > &Ver); > > bool ParseDepends(pkgCache::VerIterator &Ver, > > char **namel, char **verl, int32_t *flagl, > > > > С уважением, > Алексей Никифоров > > --1807885841-774305638-1580315073=:6363--