[devel] [SCM] packages/apt: heads/rework-dynamic-mmap
Aleksei Nikiforov
darktemplar на altlinux.org
Чт Янв 30 11:56:21 MSK 2020
Здравствуй.
29.01.2020 19:24, Ivan Zakharyaschev пишет:
> 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. Это же понятнее и
> лишних вопросов не вызвает: что потом с этой частично осмысленно
> заполненной, частично незаполненной структурой произойдёт и кто её в таком
> виде увидит дальше.
>
По сути установление нуля - это лишь сохранение старого поведения. Если
вызов WriteString(Version) возвращал ноль, то сначала этот ноль
присваивался Prv->ProvideVersion, а уже затем проверялось значение
Prv->ProvideVersion. С новым API в случае ошибки возвращается особое
значение, пустой std::experimental::optional, но для сохранения старого
поведения данной функции pkgCacheGenerator::ListParser::NewProvides ноль
присваивается переменной Prv->ProvideVersion. Я не счёл необходимым
сильнее менять данную функцию, по крайней мере до сих пор, но если есть
желание дальше изменять данный код - я не возражаю.
>> 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