[devel] [SCM] packages/apt: heads/rework-dynamic-mmap
Ivan Zakharyaschev
imz на altlinux.org
Ср Янв 29 19:24:33 MSK 2020
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<unsigned long>()
> > + : 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<pkgCache::PrvIterator> 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 <darktemplar на altlinux.org>
> > 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 <imz на altlinux.org> and
> > Vladimir D. Seleznev <vseleznv на altlinux.org>.
> >
> > 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<unsigned long>
> > 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<unsigned long>();
> > }
> > - return Result;
> > + return std::experimental::optional<unsigned long>(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<unsigned long> DynamicMMap::Allocate(unsigned
> > long ItemSize)
> > +{
> > + if (ItemSize == 0)
> > + {
> > + _error->Error("Can't allocate an item of size zero");
> > + return std::experimental::optional<unsigned long>();
> > + }
> > +
> > // 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<unsigned long>();
> > }
> >
> > 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<unsigned long>(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<unsigned long> 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<unsigned long>();
> > }
> >
> > 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<unsigned long>(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 <string>
> > +#include <experimental/optional>
> > #include <apt-pkg/fileutl.h>
> >
> > 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<unsigned long> RawAllocate(unsigned long
> > Size,unsigned long Aln = 0);
> > + std::experimental::optional<unsigned long> Allocate(unsigned long
> > ItemSize);
> > + std::experimental::optional<unsigned long> WriteString(const char
> > *String,unsigned long Len = (unsigned long)-1);
> > + inline std::experimental::optional<unsigned long> 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<char*>(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<map_ptrloc>
> > 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<map_ptrloc>
> > pkgCacheGenerator::WriteStringInMap(const char *String) {
> > return Map.WriteString(String);
> > }
> > /*}}}*/
> > -unsigned long pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
> > +std::experimental::optional<map_ptrloc>
> > 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<unsigned long>
> > 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<unsigned long>();
> > +
> > // 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<unsigned long>()
> > + : 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<unsigned long>
> > 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<unsigned long>(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<unsigned long>(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<unsigned long>();
> >
> > // 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<unsigned long>(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 <apt-pkg/pkgcache.h>
> >
> > +#include <experimental/optional>
> > +
> > 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<unsigned long> WriteStringInMap(const
> > std::string &String) { return WriteStringInMap(String.c_str(),
> > String.length()); };
> > + std::experimental::optional<unsigned long> WriteStringInMap(const char
> > *String);
> > + std::experimental::optional<unsigned long> WriteStringInMap(const char
> > *String, unsigned long Len);
> > + std::experimental::optional<unsigned long> 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<unsigned long>
> > 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<unsigned long> WriteUniqString(const char
> > *S,unsigned int Size);
> > + inline std::experimental::optional<unsigned long> 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<unsigned long> WriteUniqString(const
> > string &S) {return Owner->WriteUniqString(S);};
> > + inline std::experimental::optional<unsigned long> WriteUniqString(const
> > char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);};
> > + inline std::experimental::optional<unsigned long> WriteString(const
> > string &S) {return Owner->WriteStringInMap(S);};
> > + inline std::experimental::optional<unsigned long> 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<unsigned long>
> > 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<unsigned long>();
> >
> > 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 <map>
> > #include <vector>
> > #include <regex.h>
> > +#include <experimental/optional>
> >
> > using namespace std;
> >
> > @@ -45,7 +46,7 @@ class rpmListParser : public pkgCacheGenerator::ListParser
> >
> > bool Duplicated;
> >
> > - unsigned long UniqFindTagWrite(int Tag);
> > + std::experimental::optional<unsigned long> UniqFindTagWrite(int Tag);
> > bool ParseStatus(pkgCache::PkgIterator &Pkg,pkgCache::VerIterator
> > &Ver);
> > bool ParseDepends(pkgCache::VerIterator &Ver,
> > char **namel, char **verl, int32_t *flagl,
> >
>
> С уважением,
> Алексей Никифоров
>
>
Подробная информация о списке рассылки Devel