* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
@ 2020-01-29 16:24 ` Ivan Zakharyaschev
2020-01-29 16:32 ` Ivan Zakharyaschev
2020-01-30 8:56 ` Aleksei Nikiforov
0 siblings, 2 replies; 13+ messages in thread
From: Ivan Zakharyaschev @ 2020-01-29 16:24 UTC (permalink / raw)
To: Aleksei Nikiforov; +Cc: Anton Farygin, devel, Vladimir D. Seleznev
[-- Attachment #1: Type: text/plain, Size: 41634 bytes --]
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,
> >
>
> С уважением,
> Алексей Никифоров
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-01-29 16:24 ` [devel] [SCM] packages/apt: heads/rework-dynamic-mmap Ivan Zakharyaschev
@ 2020-01-29 16:32 ` Ivan Zakharyaschev
2020-01-30 9:30 ` Aleksei Nikiforov
2020-01-30 8:56 ` Aleksei Nikiforov
1 sibling, 1 reply; 13+ messages in thread
From: Ivan Zakharyaschev @ 2020-01-29 16:32 UTC (permalink / raw)
To: ALT Linux Team development discussions
Cc: Anton Farygin, Aleksei Nikiforov, Vladimir D. Seleznev
[-- Attachment #1: Type: text/plain, Size: 5115 bytes --]
On Wed, 29 Jan 2020, Ivan Zakharyaschev wrote:
> On Wed, 29 Jan 2020, Aleksei Nikiforov wrote:
> > Вот второй фрагмент изменений под вопросом:
> >
> > > @@ -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;
У меня возник вопрос: а в случае, если Version.empty() истинно, нужно ли
как-то заполнять Prv->ProvideVersion специально?
Раньше (то, что минусами отмечено) в таком случае Prv->ProvideVersion
никак не трогали (потому что происходил shortcut в вычислении &&).
> > Здесь мне следующее условие кажется неоправданно усложнённым и трудночитаемым
> > по сравнение с кодом из моего изменения:
> >
> > 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. Это же понятнее и
> лишних вопросов не вызвает: что потом с этой частично осмысленно
> заполненной, частично незаполненной структурой произойдёт и кто её в таком
> виде увидит дальше.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-01-29 16:24 ` [devel] [SCM] packages/apt: heads/rework-dynamic-mmap Ivan Zakharyaschev
2020-01-29 16:32 ` Ivan Zakharyaschev
@ 2020-01-30 8:56 ` Aleksei Nikiforov
2020-02-11 13:47 ` Ivan Zakharyaschev
1 sibling, 1 reply; 13+ messages in thread
From: Aleksei Nikiforov @ 2020-01-30 8:56 UTC (permalink / raw)
To: Ivan Zakharyaschev; +Cc: Anton Farygin, devel, Vladimir D. Seleznev
Здравствуй.
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,
>>>
>>
>> С уважением,
>> Алексей Никифоров
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-01-29 16:32 ` Ivan Zakharyaschev
@ 2020-01-30 9:30 ` Aleksei Nikiforov
0 siblings, 0 replies; 13+ messages in thread
From: Aleksei Nikiforov @ 2020-01-30 9:30 UTC (permalink / raw)
To: Ivan Zakharyaschev
Cc: Anton Farygin, ALT Linux Team development discussions,
Vladimir D. Seleznev
29.01.2020 19:32, Ivan Zakharyaschev пишет:
>
> On Wed, 29 Jan 2020, Ivan Zakharyaschev wrote:
>
>> On Wed, 29 Jan 2020, Aleksei Nikiforov wrote:
>
>>> Вот второй фрагмент изменений под вопросом:
>>>
>>>> @@ -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;
>
> У меня возник вопрос: а в случае, если Version.empty() истинно, нужно ли
> как-то заполнять Prv->ProvideVersion специально?
>
> Раньше (то, что минусами отмечено) в таком случае Prv->ProvideVersion
> никак не трогали (потому что происходил shortcut в вычислении &&).
>
Сложно ответить на этот вопрос, но на всякий случай выставлять особое
значение "ноль" думаю повредить не должно.
С уважением,
Алексей Никифоров
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-01-30 8:56 ` Aleksei Nikiforov
@ 2020-02-11 13:47 ` Ivan Zakharyaschev
2020-02-11 13:51 ` [devel] std::optional; was: " Ivan Zakharyaschev
2020-02-11 14:00 ` [devel] [SCM] packages/apt: heads/rework-dynamic-mmap Dmitry V. Levin
0 siblings, 2 replies; 13+ messages in thread
From: Ivan Zakharyaschev @ 2020-02-11 13:47 UTC (permalink / raw)
To: ALT Linux Team development discussions
Cc: Anton Farygin, Vladimir D. Seleznev
[-- Attachment #1: Type: text/plain, Size: 39198 bytes --]
Hello!
On Thu, 30 Jan 2020, Aleksei Nikiforov wrote:
> > Чтобы не было удивления от бессмысленного значения, я переписал так, как
> > ты увидел. Не люблю операции, которые какой-то мусор присваивают, просто
> > чтобы место чем-то заполнить.
> >
> > В моём варианте Prv даже не конструируется и не начинает заполняться, если
> > не удалось выделить память в Map под строку Version. Это же понятнее и
> > лишних вопросов не вызвает: что потом с этой частично осмысленно
> > заполненной, частично незаполненной структурой произойдёт и кто её в таком
> > виде увидит дальше.
> >
>
> По сути установление нуля - это лишь сохранение старого поведения. Если вызов
> WriteString(Version) возвращал ноль, то сначала этот ноль присваивался
> Prv->ProvideVersion, а уже затем проверялось значение Prv->ProvideVersion. С
> новым API в случае ошибки возвращается особое значение, пустой
> std::experimental::optional, но для сохранения старого поведения данной
> функции pkgCacheGenerator::ListParser::NewProvides ноль присваивается
> переменной Prv->ProvideVersion. Я не счёл необходимым сильнее менять данную
> функцию, по крайней мере до сих пор, но если есть желание дальше изменять
> данный код - я не возражаю.
Теперь в своей ветке optional учёл это высказанное соображение и прочие
мелкие замечания, которые тут высказывались к моим редакциям этих
изменений в виде коммитов.
В этом месте сначала делаю ближе к старому поведению, потом в другом
коммите меняю сильнее.
Теперь вся серия выглядит так:
$ git --no-pager log -p base..optional
commit 9146fbb5ccf224505334190472fe7ddfc1531e4e
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Mon Feb 10 13:03:07 2020 +0300
.dir-locals.el: apply our Emacs mode settings to .h files, too
diff --git a/apt/.dir-locals.el b/apt/.dir-locals.el
index c1497844e..acc107299 100644
--- a/apt/.dir-locals.el
+++ b/apt/.dir-locals.el
@@ -2,4 +2,9 @@
;;; For more information see (info "(emacs) Directory Variables")
((c++-mode
+ (c-basic-offset . 3))
+
+ ;; For switching in .h headers:
+ (c-mode
+ (mode . c++)
(c-basic-offset . 3)))
commit f9de67b18e05656ec43df41cde61dbb338fd01f4
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Mon Feb 10 17:02:20 2020 +0300
DynamicMMap::RawAllocate(): do not modify data on failure
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index ef9e31242..d387560e8 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -207,9 +207,7 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln)
unsigned long long Result = iSize;
if (Aln != 0)
Result += Aln - (iSize%Aln);
-
- iSize = Result + Size;
-
+
// Just in case error check
if (Result + Size > WorkSpace)
{
@@ -217,6 +215,8 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln)
return 0;
}
+ iSize = Result + Size;
+
return Result;
}
/*}}}*/
commit 5fa3ac151d7b5290a775e69f7e79dce0837db2d8
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Mon Feb 10 17:03:42 2020 +0300
DynamicMMap::RawAllocate(): no offset needed if already aligned (was off-by-one)
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index d387560e8..44cc6bae4 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -206,7 +206,7 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln)
{
unsigned long long Result = iSize;
if (Aln != 0)
- Result += Aln - (iSize%Aln);
+ Result += Aln - (iSize%Aln ? : Aln);
// Just in case error check
if (Result + Size > WorkSpace)
commit ca6142850ccea2f726bb59295f16fe5a0820a562
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Mon Feb 10 19:29:34 2020 +0300
DynamicMMap::WriteString(): fix the check for "out of room" when length is automatic
It made no sense to make the check before we have the valid value in Len.
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index 44cc6bae4..6b56775a1 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -271,6 +271,9 @@ unsigned long DynamicMMap::Allocate(unsigned long ItemSize)
unsigned long DynamicMMap::WriteString(const char *String,
unsigned long Len)
{
+ if (Len == (unsigned long)-1)
+ Len = strlen(String);
+
unsigned long Result = iSize;
// Just in case error check
if (Result + Len > WorkSpace)
@@ -279,8 +282,6 @@ unsigned long DynamicMMap::WriteString(const char *String,
return 0;
}
- if (Len == (unsigned long)-1)
- Len = strlen(String);
iSize += Len + 1;
memcpy((char *)Base + Result,String,Len);
((char *)Base)[Result + Len] = 0;
commit e785f0e8636e47a672445e70f2923a5eea566b33
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Wed Jan 29 04:41:13 2020 +0300
use the safer C++-style static_cast instead of a C-style cast (from void*)
What is happening here:
Map->RawAllocate() returns the index in an array of bytes (i.e., of char;
no matter whether they are (un)signed); therefore, we cast the base
pointer to the corresponding type, so that the pointer arithmetic
gives a pointer to the beginning of the allocated space.
We do not want to rely on non-standard void*-arithmetic.
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 070c0f040..16d3592e9 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -882,7 +882,7 @@ 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()),
+ if (SCacheF.Read(static_cast<char *>(Map->Data()) + Map->RawAllocate(SCacheF.Size()),
SCacheF.Size()) == false)
return false;
commit 4d297001f6d3ddc3e12cf4b950e821d867b77092
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Wed Jan 29 05:02:13 2020 +0300
rigorously compute HeaderP as the pointer to the beginning of Map
The old code expected that the first call to Map.RawAllocate() would
give the pointer to the very beginning of Map, i.e., the same as
Map.Data().
We rewrite this code without such presupposition: no matter what the
logic behind Map.RawAllocate() will ever be, we'll actually get the
pointer to the space which is being "allocated" by this call inside
Map.
We use the safer C++-style static_cast (from void*) where possible.
What is happening here with the pointer arithmetic:
Map->RawAllocate() returns the index in an array of bytes (i.e., of char);
therefore, we cast the base pointer to the corresponding type, so that
the pointer arithmetic gives the correct pointer to the beginning of the
"allocated" space.
We do not want to rely on non-standard void*-arithmetic.
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 16d3592e9..8bf5ed639 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -54,9 +54,10 @@ pkgCacheGenerator::pkgCacheGenerator(DynamicMMap *pMap,OpProgress *Prog) :
if (Map.Size() == 0)
{
+ const auto idxBeginning = Map.RawAllocate(sizeof(pkgCache::Header));
+
// Setup the map interface..
- Cache.HeaderP = (pkgCache::Header *)Map.Data();
- Map.RawAllocate(sizeof(pkgCache::Header));
+ Cache.HeaderP = reinterpret_cast<pkgCache::Header *>(static_cast<char *>(Map.Data()) + idxBeginning);
Map.UsePools(*Cache.HeaderP->Pools,sizeof(Cache.HeaderP->Pools)/sizeof(Cache.HeaderP->Pools[0]));
// Starting header
commit 063c9051ddc8d736193733f19ab9a821c30ce3b8
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Wed Jan 29 20:41:33 2020 +0300
Use special type to return allocation failure since 0 is a valid offset value
This change is heavily based on:
commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6
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
which has been rebased and purified a bit (so that the code clearly
matches the intention at the changed places).
Co-Authored-by: Vladimir D. Seleznev <vseleznv@altlinux.org>.
diff --git a/apt.spec b/apt.spec
index 362179ec1..4b18a9453 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 6b56775a1..77aad3656 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)
@@ -212,7 +212,7 @@ 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::nullopt;
}
iSize = Result + Size;
@@ -224,8 +224,8 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln)
// ---------------------------------------------------------------------
/* 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)
+{
// Look for a matching pool entry
Pool *I;
Pool *Empty = 0;
@@ -244,7 +244,7 @@ unsigned long DynamicMMap::Allocate(unsigned long ItemSize)
if (Empty == 0)
{
_error->Error("Ran out of allocation pools");
- return 0;
+ return std::experimental::nullopt;
}
I = Empty;
@@ -256,8 +256,14 @@ 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);
+
+ // Has the allocation failed?
+ if (!idxResult)
+ return idxResult;
+
+ I->Start = *idxResult;
+ }
I->Count--;
unsigned long Result = I->Start;
@@ -268,7 +274,7 @@ unsigned long DynamicMMap::Allocate(unsigned long 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)
{
if (Len == (unsigned long)-1)
@@ -279,7 +285,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::nullopt;
}
iSize += Len + 1;
diff --git a/apt/apt-pkg/contrib/mmap.h b/apt/apt-pkg/contrib/mmap.h
index efc5245cb..14df0550c 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 8bf5ed639..4c6055cbb 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -55,16 +55,20 @@ pkgCacheGenerator::pkgCacheGenerator(DynamicMMap *pMap,OpProgress *Prog) :
if (Map.Size() == 0)
{
const auto idxBeginning = Map.RawAllocate(sizeof(pkgCache::Header));
+ const auto idxVerSysName = WriteStringInMap(_system->VS->Label);
+ const auto idxArch = WriteStringInMap(_config->Find("APT::Architecture"));
+ if ((!idxBeginning) || (!idxVerSysName) || (!idxArch))
+ return;
// Setup the map interface..
- Cache.HeaderP = reinterpret_cast<pkgCache::Header *>(static_cast<char *>(Map.Data()) + idxBeginning);
+ Cache.HeaderP = reinterpret_cast<pkgCache::Header *>(static_cast<char *>(Map.Data()) + *idxBeginning);
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
{
@@ -98,17 +102,17 @@ pkgCacheGenerator::~pkgCacheGenerator()
}
/*}}}*/
// CacheGenerator::WriteStringInMap /*{{{*/
-unsigned long pkgCacheGenerator::WriteStringInMap(const char *String,
+std::experimental::optional<unsigned long> 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<unsigned long> pkgCacheGenerator::WriteStringInMap(const char *String) {
return Map.WriteString(String);
}
/*}}}*/
-unsigned long pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
+std::experimental::optional<unsigned long> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
return Map.Allocate(size);
}
/*}}}*/
@@ -222,7 +226,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)
@@ -366,21 +375,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;
@@ -408,7 +416,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;
}
}
@@ -416,11 +428,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
@@ -442,23 +454,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::nullopt;
+
// 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;
}
/*}}}*/
@@ -475,12 +486,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;
@@ -498,8 +509,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
@@ -545,18 +561,29 @@ 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));
+ 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);
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);
+ // FIXME: If this allocation fails, did it make sense to fill in Prv above?
+ if (!idxVersion)
+ {
+ // FIXME: Does it really make sense to fill it in on failure?
+ Prv->ProvideVersion = 0;
+ return false;
+ }
+ Prv->ProvideVersion = *idxVersion;
+ }
// Locate the target package
pkgCache::PkgIterator Pkg;
@@ -580,23 +607,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());
@@ -607,7 +635,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
@@ -637,18 +665,17 @@ unsigned long pkgCacheGenerator::WriteUniqString(const char *S,
}
// 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::nullopt;
// 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;
}
@@ -883,7 +910,10 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress,
{
// Preload the map with the source cache
FileFd SCacheF(SrcCacheFile,FileFd::ReadOnly);
- if (SCacheF.Read(static_cast<char *>(Map->Data()) + Map->RawAllocate(SCacheF.Size()),
+ const auto idxAllocate = Map->RawAllocate(SCacheF.Size());
+ if (!idxAllocate)
+ return false;
+ if (SCacheF.Read(static_cast<char *>(Map->Data()) + *idxAllocate,
SCacheF.Size()) == false)
return false;
diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
index a187cd1e8..69c4f2ea1 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/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
index 2fc4e01b8..b3d18d7e6 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::nullopt;
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 5c6173c0a..dd57f7c9b 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,
commit 555697737ef0f0280ba7521fe788ce8b4ebab492
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Thu Jan 30 15:40:00 2020 +0300
don't even start to fill in a structure, if we can't allocate the prerequisites
FIXME: There are other similar placase in this code; have a look at
them and possibly re-organize. (E.g.,
pkgCacheGenerator::ListParser::NewDepends(), apart from
pkgCacheGenerator::ListParser::NewProvides() already changed here.)
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 4c6055cbb..fe0a4e86e 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -559,11 +559,27 @@ bool pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver,
if (Ver.ParentPkg().Name() == PackageName)
return true;
#endif
-
+
+ // used only if !Version.empty()
+ std::experimental::optional<unsigned long> idxVersion = std::experimental::nullopt;
+ // Try to allocate the prerequisite string
+ if (Version.empty() == false) {
+ idxVersion = WriteString(Version);
+ if (!idxVersion)
+ {
+ /* Don't even start to fill in the structure below,
+ if we can't get the prerequisite. */
+ return false;
+ }
+ }
// Get a structure
const auto Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides));
if (!Provides)
return false;
+ /* Note:
+ this accounting is done only if all the prerequistes have been allocated
+ and hence the structure will actually be used (filled in).
+ */
Cache.HeaderP->ProvidesCount++;
// Fill it in
@@ -572,16 +588,10 @@ bool pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver,
Prv->NextPkgProv = Ver->ProvidesList;
Ver->ProvidesList = Prv.Index();
- if (Version.empty() == false)
+ if (idxVersion)
{
- const auto idxVersion = WriteString(Version);
- // FIXME: If this allocation fails, did it make sense to fill in Prv above?
- if (!idxVersion)
- {
- // FIXME: Does it really make sense to fill it in on failure?
- Prv->ProvideVersion = 0;
- return false;
- }
+ /* FIXME: If Version.empty(), this field has never been touched.
+ Is this correct? */
Prv->ProvideVersion = *idxVersion;
}
commit b06f43d3c951ba7b2da8108516b551f1207beed6
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Wed Jan 29 21:40:53 2020 +0300
DynamicMMap::RawAllocate(): error on "Can't allocate an item of size zero"
Picked this single change from:
commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b
Author: Aleksei Nikiforov <darktemplar@altlinux.org>
Date: Tue Jun 11 14:53:47 2019 +0300
apt-pkg/pkgcachegen.{cc,h} changes
make the used MMap moveable (and therefore dynamic resizeable) by
applying (some) mad pointer magic
Ported from Apt from Debian
and adapted to the new return type.
diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
index 77aad3656..ad49326df 100644
--- a/apt/apt-pkg/contrib/mmap.cc
+++ b/apt/apt-pkg/contrib/mmap.cc
@@ -226,6 +226,12 @@ std::experimental::optional<unsigned long> DynamicMMap::RawAllocate(unsigned lon
size in the file. */
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::nullopt;
+ }
+
// Look for a matching pool entry
Pool *I;
Pool *Empty = 0;
commit b92df3199a5eda44f5108e6721c5cb76d946d0fc
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Thu Jan 30 01:28:56 2020 +0300
rpmindexfile.cc rpmPkgListIndex::Merge(): drop a bogus call to FileFd::Seek()
This operation couldn't have any noticeable effect, because
the corresponding FileFd object is about to be destroyed: it's the end of
its scope. (Note that it was declared and constructed 4 lines above.
This explanation was given by darktemplar@ and quoted in
https://lists.altlinux.org/pipermail/devel/2020-January/209736.html .)
This single change has been picked from the following changeset (which
otherwise has a different common purpose):
commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6
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
This change will slightly alleviate the rewriting of code at this
place for rigorous error-handling (which is a change also present in
the cited-above commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6, but
not related to its purpose).
diff --git a/apt/apt-pkg/rpm/rpmindexfile.cc b/apt/apt-pkg/rpm/rpmindexfile.cc
index 271cd8f83..a66d4482c 100644
--- a/apt/apt-pkg/rpm/rpmindexfile.cc
+++ b/apt/apt-pkg/rpm/rpmindexfile.cc
@@ -407,7 +407,6 @@ bool rpmPkgListIndex::Merge(pkgCacheGenerator &Gen,OpProgress &Prog) const
if (_error->PendingError() == true)
return false;
Parser.LoadReleaseInfo(File,Rel);
- Rel.Seek(0);
}
return true;
commit 7db04197e0029146ace692a4fa9ee7088dd76b8a (@ALT/optional, optional)
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Thu Jan 30 04:44:06 2020 +0300
rpmindexfile.cc rpmPkgListIndex::Merge(): rigorously handle errors from Parser
This single change has been picked from the following changeset (which
otherwise has a different common purpose):
commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6
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
Unfortunately, that commit doesn't explain why the new behavior is
better than the old one (ignoring the error from Parser).
diff --git a/apt/apt-pkg/rpm/rpmindexfile.cc b/apt/apt-pkg/rpm/rpmindexfile.cc
index a66d4482c..5ba42155c 100644
--- a/apt/apt-pkg/rpm/rpmindexfile.cc
+++ b/apt/apt-pkg/rpm/rpmindexfile.cc
@@ -406,7 +406,8 @@ bool rpmPkgListIndex::Merge(pkgCacheGenerator &Gen,OpProgress &Prog) const
FileFd Rel(RelFile,FileFd::ReadOnly);
if (_error->PendingError() == true)
return false;
- Parser.LoadReleaseInfo(File,Rel);
+ if (!Parser.LoadReleaseInfo(File,Rel))
+ return false;
}
return true;
--
Best regards,
Ivan
^ permalink raw reply [flat|nested] 13+ messages in thread
* [devel] std::optional; was: Re: [SCM] packages/apt: heads/rework-dynamic-mmap
2020-02-11 13:47 ` Ivan Zakharyaschev
@ 2020-02-11 13:51 ` Ivan Zakharyaschev
2020-02-11 20:06 ` [devel] std::optional Dmitry V. Levin
2020-02-11 14:00 ` [devel] [SCM] packages/apt: heads/rework-dynamic-mmap Dmitry V. Levin
1 sibling, 1 reply; 13+ messages in thread
From: Ivan Zakharyaschev @ 2020-02-11 13:51 UTC (permalink / raw)
To: ALT Linux Team development discussions
Cc: Anton Farygin, Vladimir D. Seleznev
[-- Attachment #1: Type: text/plain, Size: 428 bytes --]
Как думаете: может, будет читабельнее, если мы в коде напишем просто
std::optional и #include<optional>
а перед сборкой на e2k с помощью lcc будем sed-ом вставлять experimental?
Всё равно рано или поздно и другие компиляторы (помимо gcc) будут
поддерживать такой стандарт, где оно уже не optional.
Без лишнего слова experimental в патчах код будт более готов к тому, чтобы
быть красивым в будущем.
--
Best regards,
Ivan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-02-11 13:47 ` Ivan Zakharyaschev
2020-02-11 13:51 ` [devel] std::optional; was: " Ivan Zakharyaschev
@ 2020-02-11 14:00 ` Dmitry V. Levin
2020-02-11 14:24 ` Ivan Zakharyaschev
1 sibling, 1 reply; 13+ messages in thread
From: Dmitry V. Levin @ 2020-02-11 14:00 UTC (permalink / raw)
To: ALT Devel discussion list
On Tue, Feb 11, 2020 at 04:47:41PM +0300, Ivan Zakharyaschev wrote:
[...]
> commit e785f0e8636e47a672445e70f2923a5eea566b33
> Author: Ivan Zakharyaschev <imz@altlinux.org>
> Date: Wed Jan 29 04:41:13 2020 +0300
>
> use the safer C++-style static_cast instead of a C-style cast (from void*)
>
> What is happening here:
>
> Map->RawAllocate() returns the index in an array of bytes (i.e., of char;
> no matter whether they are (un)signed); therefore, we cast the base
> pointer to the corresponding type, so that the pointer arithmetic
> gives a pointer to the beginning of the allocated space.
>
> We do not want to rely on non-standard void*-arithmetic.
We - это кто, и почему они не хотят полагаться на то, что работает?
--
ldv
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-02-11 14:00 ` [devel] [SCM] packages/apt: heads/rework-dynamic-mmap Dmitry V. Levin
@ 2020-02-11 14:24 ` Ivan Zakharyaschev
2020-02-11 17:05 ` Ivan Zakharyaschev
0 siblings, 1 reply; 13+ messages in thread
From: Ivan Zakharyaschev @ 2020-02-11 14:24 UTC (permalink / raw)
To: ALT Linux Team development discussions
[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]
On Tue, 11 Feb 2020, Dmitry V. Levin wrote:
> On Tue, Feb 11, 2020 at 04:47:41PM +0300, Ivan Zakharyaschev wrote:
> [...]
> > commit e785f0e8636e47a672445e70f2923a5eea566b33
> > Author: Ivan Zakharyaschev <imz@altlinux.org>
> > Date: Wed Jan 29 04:41:13 2020 +0300
> >
> > use the safer C++-style static_cast instead of a C-style cast (from void*)
> >
> > What is happening here:
> >
> > Map->RawAllocate() returns the index in an array of bytes (i.e., of char;
> > no matter whether they are (un)signed); therefore, we cast the base
> > pointer to the corresponding type, so that the pointer arithmetic
> > gives a pointer to the beginning of the allocated space.
> >
> > We do not want to rely on non-standard void*-arithmetic.
>
> We - это кто, и почему они не хотят полагаться на то, что работает?
Да это расширение gcc, которое работает , когда язык C, а когда C++ --
запрещено и в gcc. (Я вроде пробовал, и gcc запретил, когда мы в прошлый
раз обсуждали падение apt-а на e2k.)
--
Best regards,
Ivan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-02-11 14:24 ` Ivan Zakharyaschev
@ 2020-02-11 17:05 ` Ivan Zakharyaschev
2020-02-11 18:14 ` Dmitry V. Levin
0 siblings, 1 reply; 13+ messages in thread
From: Ivan Zakharyaschev @ 2020-02-11 17:05 UTC (permalink / raw)
To: ALT Linux Team development discussions
[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]
On Tue, 11 Feb 2020, Ivan Zakharyaschev wrote:
> On Tue, 11 Feb 2020, Dmitry V. Levin wrote:
>
> > On Tue, Feb 11, 2020 at 04:47:41PM +0300, Ivan Zakharyaschev wrote:
> > [...]
> > > commit e785f0e8636e47a672445e70f2923a5eea566b33
> > > Author: Ivan Zakharyaschev <imz@altlinux.org>
> > > Date: Wed Jan 29 04:41:13 2020 +0300
> > >
> > > use the safer C++-style static_cast instead of a C-style cast (from void*)
> > >
> > > What is happening here:
> > >
> > > Map->RawAllocate() returns the index in an array of bytes (i.e., of char;
> > > no matter whether they are (un)signed); therefore, we cast the base
> > > pointer to the corresponding type, so that the pointer arithmetic
> > > gives a pointer to the beginning of the allocated space.
> > >
> > > We do not want to rely on non-standard void*-arithmetic.
> >
> > We - это кто, и почему они не хотят полагаться на то, что работает?
>
> Да это расширение gcc, которое работает , когда язык C, а когда C++ --
> запрещено и в gcc. (Я вроде пробовал, и gcc запретил, когда мы в прошлый
> раз обсуждали падение apt-а на e2k.)
-bash-4.3$ cat charp-arith.cxx
#include <stddef.h>
#include <stdio.h>
char buf[] = "foo";
int main(const int argc, char ** const argv)
{
char * const begin = &buf[0];
char * const end = &buf[2];
const ptrdiff_t diff = end - begin;
printf("%td\n", diff);
}
-bash-4.3$ gcc -Wall -xc charp-arith.cxx && ./a.out; echo $?
2
0
-bash-4.3$ gcc -Wall charp-arith.cxx && ./a.out; echo $?
2
0
-bash-4.3$ gcc -Wall -std=gnu++17 charp-arith.cxx && ./a.out; echo $?
2
0
-bash-4.3$ diff charp-arith.cxx voidp-arith.cxx
8,9c8,9
< char * const begin = &buf[0];
< char * const end = &buf[2];
---
> void * const begin = &buf[0];
> void * const end = &buf[2];
-bash-4.3$ gcc -Wall -xc voidp-arith.cxx && ./a.out; echo $?
2
0
-bash-4.3$ gcc -Wall voidp-arith.cxx && ./a.out; echo $?
voidp-arith.cxx: In function ‘int main(int, char**)’:
voidp-arith.cxx:10:32: error: invalid use of ‘void’
10 | const ptrdiff_t diff = end - begin;
| ^~~~~
1
-bash-4.3$ gcc -Wall -std=gnu++17 voidp-arith.cxx && ./a.out; echo $?
voidp-arith.cxx: In function ‘int main(int, char**)’:
voidp-arith.cxx:10:32: error: invalid use of ‘void’
10 | const ptrdiff_t diff = end - begin;
| ^~~~~
1
-bash-4.3$ gcc -Wall -std=gnu++2a voidp-arith.cxx && ./a.out; echo $?
voidp-arith.cxx: In function ‘int main(int, char**)’:
voidp-arith.cxx:10:32: error: invalid use of ‘void’
10 | const ptrdiff_t diff = end - begin;
| ^~~~~
1
-bash-4.3$
--
Best regards,
Ivan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-02-11 17:05 ` Ivan Zakharyaschev
@ 2020-02-11 18:14 ` Dmitry V. Levin
2020-02-12 2:14 ` Ivan Zakharyaschev
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry V. Levin @ 2020-02-11 18:14 UTC (permalink / raw)
To: ALT Devel discussion list
On Tue, Feb 11, 2020 at 08:05:38PM +0300, Ivan Zakharyaschev wrote:
> On Tue, 11 Feb 2020, Ivan Zakharyaschev wrote:
>
> > On Tue, 11 Feb 2020, Dmitry V. Levin wrote:
> >
> > > On Tue, Feb 11, 2020 at 04:47:41PM +0300, Ivan Zakharyaschev wrote:
> > > [...]
> > > > commit e785f0e8636e47a672445e70f2923a5eea566b33
> > > > Author: Ivan Zakharyaschev <imz@altlinux.org>
> > > > Date: Wed Jan 29 04:41:13 2020 +0300
> > > >
> > > > use the safer C++-style static_cast instead of a C-style cast (from void*)
> > > >
> > > > What is happening here:
> > > >
> > > > Map->RawAllocate() returns the index in an array of bytes (i.e., of char;
> > > > no matter whether they are (un)signed); therefore, we cast the base
> > > > pointer to the corresponding type, so that the pointer arithmetic
> > > > gives a pointer to the beginning of the allocated space.
> > > >
> > > > We do not want to rely on non-standard void*-arithmetic.
> > >
> > > We - это кто, и почему они не хотят полагаться на то, что работает?
> >
> > Да это расширение gcc, которое работает , когда язык C, а когда C++ --
> > запрещено и в gcc. (Я вроде пробовал, и gcc запретил, когда мы в прошлый
> > раз обсуждали падение apt-а на e2k.)
>
> -bash-4.3$ cat charp-arith.cxx
> #include <stddef.h>
> #include <stdio.h>
>
> char buf[] = "foo";
>
> int main(const int argc, char ** const argv)
> {
> char * const begin = &buf[0];
> char * const end = &buf[2];
> const ptrdiff_t diff = end - begin;
> printf("%td\n", diff);
> }
> -bash-4.3$ gcc -Wall -xc charp-arith.cxx && ./a.out; echo $?
> 2
> 0
> -bash-4.3$ gcc -Wall charp-arith.cxx && ./a.out; echo $?
> 2
> 0
> -bash-4.3$ gcc -Wall -std=gnu++17 charp-arith.cxx && ./a.out; echo $?
> 2
> 0
> -bash-4.3$ diff charp-arith.cxx voidp-arith.cxx
> 8,9c8,9
> < char * const begin = &buf[0];
> < char * const end = &buf[2];
> ---
> > void * const begin = &buf[0];
> > void * const end = &buf[2];
> -bash-4.3$ gcc -Wall -xc voidp-arith.cxx && ./a.out; echo $?
> 2
> 0
> -bash-4.3$ gcc -Wall voidp-arith.cxx && ./a.out; echo $?
> voidp-arith.cxx: In function ‘int main(int, char**)’:
> voidp-arith.cxx:10:32: error: invalid use of ‘void’
> 10 | const ptrdiff_t diff = end - begin;
> | ^~~~~
> 1
> -bash-4.3$ gcc -Wall -std=gnu++17 voidp-arith.cxx && ./a.out; echo $?
> voidp-arith.cxx: In function ‘int main(int, char**)’:
> voidp-arith.cxx:10:32: error: invalid use of ‘void’
> 10 | const ptrdiff_t diff = end - begin;
> | ^~~~~
> 1
> -bash-4.3$ gcc -Wall -std=gnu++2a voidp-arith.cxx && ./a.out; echo $?
> voidp-arith.cxx: In function ‘int main(int, char**)’:
> voidp-arith.cxx:10:32: error: invalid use of ‘void’
> 10 | const ptrdiff_t diff = end - begin;
> | ^~~~~
> 1
> -bash-4.3$
Если арифметика с void* в cxx не поддерживается, значит, она не
использовалась, и фраза "We do not want to rely on non-standard
void*-arithmetic" просто сбивает с толку.
C-style cast - это не арифметика с указателями.
--
ldv
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] std::optional
2020-02-11 13:51 ` [devel] std::optional; was: " Ivan Zakharyaschev
@ 2020-02-11 20:06 ` Dmitry V. Levin
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry V. Levin @ 2020-02-11 20:06 UTC (permalink / raw)
To: ALT Devel discussion list
On Tue, Feb 11, 2020 at 04:51:11PM +0300, Ivan Zakharyaschev wrote:
> Как думаете: может, будет читабельнее, если мы в коде напишем просто
> std::optional и #include<optional>
>
> а перед сборкой на e2k с помощью lcc будем sed-ом вставлять experimental?
>
> Всё равно рано или поздно и другие компиляторы (помимо gcc) будут
> поддерживать такой стандарт, где оно уже не optional.
>
> Без лишнего слова experimental в патчах код будт более готов к тому, чтобы
> быть красивым в будущем.
std::optional выглядит короче и понятнее, чем std::experimental::optional,
я не против.
--
ldv
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-02-11 18:14 ` Dmitry V. Levin
@ 2020-02-12 2:14 ` Ivan Zakharyaschev
2020-02-12 8:47 ` Dmitry V. Levin
0 siblings, 1 reply; 13+ messages in thread
From: Ivan Zakharyaschev @ 2020-02-12 2:14 UTC (permalink / raw)
To: ALT Linux Team development discussions
[-- Attachment #1: Type: text/plain, Size: 3912 bytes --]
> Если арифметика с void* в cxx не поддерживается, значит, она не
> использовалась, и фраза "We do not want to rely on non-standard
> void*-arithmetic" просто сбивает с толку.
>
> C-style cast - это не арифметика с указателями.
Переписал так эти места:
commit 99994eb9e7580e9dd0014fe9a37bc68a0b9fff5e
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Wed Jan 29 04:41:13 2020 +0300
use the safer C++-style static_cast instead of a C-style cast (from void*)
What is happening here
(a commentary clarifying the code, not this particular stylistic change):
Map->RawAllocate() returns the index in an array of bytes (i.e., of char;
no matter whether they are (un)signed); therefore, we cast the base
pointer to the corresponding type, so that the pointer arithmetic
gives a pointer to the beginning of the allocated space.
If you wonder why use a cast at all here, then note that Map->Data()
returns a void* value, and pointer arithmetic on void* is not allowed in C++
(and is a non-standard extension in GNU C, not GNU C++, which would actually
give the same result as our code).
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 070c0f040..16d3592e9 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -882,7 +882,7 @@ 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()),
+ if (SCacheF.Read(static_cast<char *>(Map->Data()) + Map->RawAllocate(SCacheF.Size()),
SCacheF.Size()) == false)
return false;
commit e1db00879ce008555a6fbc3195b3cd92e6c25eb9
Author: Ivan Zakharyaschev <imz@altlinux.org>
Date: Wed Jan 29 05:02:13 2020 +0300
rigorously compute HeaderP as the pointer to the beginning of Map
The old code expected that the first call to Map.RawAllocate() would
give the pointer to the very beginning of Map, i.e., the same as
Map.Data().
We rewrite this code without such presupposition: no matter what the
logic behind Map.RawAllocate() will ever be, we'll actually get the
pointer to the space which is being "allocated" by this call inside
Map.
We use the safer C++-style static_cast (from void*) where possible.
What is happening here with the pointer arithmetic:
Map->RawAllocate() returns the index in an array of bytes (i.e., of char);
therefore, we cast the base pointer to the corresponding type, so that
the pointer arithmetic gives the correct pointer to the beginning of the
"allocated" space.
If you wonder why use a cast at all here, then note that Map->Data()
returns a void* value, and pointer arithmetic on void* is not allowed in C++
(and is a non-standard extension in GNU C, not GNU C++, which would actually
give the same result as our code).
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 16d3592e9..8bf5ed639 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -54,9 +54,10 @@ pkgCacheGenerator::pkgCacheGenerator(DynamicMMap *pMap,OpProgress *Prog) :
if (Map.Size() == 0)
{
+ const auto idxBeginning = Map.RawAllocate(sizeof(pkgCache::Header));
+
// Setup the map interface..
- Cache.HeaderP = (pkgCache::Header *)Map.Data();
- Map.RawAllocate(sizeof(pkgCache::Header));
+ Cache.HeaderP = reinterpret_cast<pkgCache::Header *>(static_cast<char *>(Map.Data()) + idxBeginning);
Map.UsePools(*Cache.HeaderP->Pools,sizeof(Cache.HeaderP->Pools)/sizeof(Cache.HeaderP->Pools[0]));
// Starting header
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap
2020-02-12 2:14 ` Ivan Zakharyaschev
@ 2020-02-12 8:47 ` Dmitry V. Levin
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry V. Levin @ 2020-02-12 8:47 UTC (permalink / raw)
To: ALT Devel discussion list
On Wed, Feb 12, 2020 at 05:14:07AM +0300, Ivan Zakharyaschev wrote:
> > Если арифметика с void* в cxx не поддерживается, значит, она не
> > использовалась, и фраза "We do not want to rely on non-standard
> > void*-arithmetic" просто сбивает с толку.
> >
> > C-style cast - это не арифметика с указателями.
>
> Переписал так эти места:
>
> commit 99994eb9e7580e9dd0014fe9a37bc68a0b9fff5e
> Author: Ivan Zakharyaschev <imz@altlinux.org>
> Date: Wed Jan 29 04:41:13 2020 +0300
>
> use the safer C++-style static_cast instead of a C-style cast (from void*)
>
> What is happening here
> (a commentary clarifying the code, not this particular stylistic change):
>
> Map->RawAllocate() returns the index in an array of bytes (i.e., of char;
> no matter whether they are (un)signed); therefore, we cast the base
> pointer to the corresponding type, so that the pointer arithmetic
> gives a pointer to the beginning of the allocated space.
>
> If you wonder why use a cast at all here, then note that Map->Data()
> returns a void* value, and pointer arithmetic on void* is not allowed in C++
> (and is a non-standard extension in GNU C, not GNU C++, which would actually
> give the same result as our code).
>
> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
> index 070c0f040..16d3592e9 100644
> --- a/apt/apt-pkg/pkgcachegen.cc
> +++ b/apt/apt-pkg/pkgcachegen.cc
> @@ -882,7 +882,7 @@ 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()),
> + if (SCacheF.Read(static_cast<char *>(Map->Data()) + Map->RawAllocate(SCacheF.Size()),
> SCacheF.Size()) == false)
> return false;
>
>
> commit e1db00879ce008555a6fbc3195b3cd92e6c25eb9
> Author: Ivan Zakharyaschev <imz@altlinux.org>
> Date: Wed Jan 29 05:02:13 2020 +0300
>
> rigorously compute HeaderP as the pointer to the beginning of Map
>
> The old code expected that the first call to Map.RawAllocate() would
> give the pointer to the very beginning of Map, i.e., the same as
> Map.Data().
>
> We rewrite this code without such presupposition: no matter what the
> logic behind Map.RawAllocate() will ever be, we'll actually get the
> pointer to the space which is being "allocated" by this call inside
> Map.
>
> We use the safer C++-style static_cast (from void*) where possible.
>
> What is happening here with the pointer arithmetic:
>
> Map->RawAllocate() returns the index in an array of bytes (i.e., of char);
> therefore, we cast the base pointer to the corresponding type, so that
> the pointer arithmetic gives the correct pointer to the beginning of the
> "allocated" space.
>
> If you wonder why use a cast at all here, then note that Map->Data()
> returns a void* value, and pointer arithmetic on void* is not allowed in C++
> (and is a non-standard extension in GNU C, not GNU C++, which would actually
> give the same result as our code).
>
> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
> index 16d3592e9..8bf5ed639 100644
> --- a/apt/apt-pkg/pkgcachegen.cc
> +++ b/apt/apt-pkg/pkgcachegen.cc
> @@ -54,9 +54,10 @@ pkgCacheGenerator::pkgCacheGenerator(DynamicMMap *pMap,OpProgress *Prog) :
>
> if (Map.Size() == 0)
> {
> + const auto idxBeginning = Map.RawAllocate(sizeof(pkgCache::Header));
> +
> // Setup the map interface..
> - Cache.HeaderP = (pkgCache::Header *)Map.Data();
> - Map.RawAllocate(sizeof(pkgCache::Header));
> + Cache.HeaderP = reinterpret_cast<pkgCache::Header *>(static_cast<char *>(Map.Data()) + idxBeginning);
> Map.UsePools(*Cache.HeaderP->Pools,sizeof(Cache.HeaderP->Pools)/sizeof(Cache.HeaderP->Pools[0]));
>
> // Starting header
Теперь понятно, спасибо.
--
ldv
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-02-12 8:47 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-29 16:24 ` [devel] [SCM] packages/apt: heads/rework-dynamic-mmap Ivan Zakharyaschev
2020-01-29 16:32 ` Ivan Zakharyaschev
2020-01-30 9:30 ` Aleksei Nikiforov
2020-01-30 8:56 ` Aleksei Nikiforov
2020-02-11 13:47 ` Ivan Zakharyaschev
2020-02-11 13:51 ` [devel] std::optional; was: " Ivan Zakharyaschev
2020-02-11 20:06 ` [devel] std::optional Dmitry V. Levin
2020-02-11 14:00 ` [devel] [SCM] packages/apt: heads/rework-dynamic-mmap Dmitry V. Levin
2020-02-11 14:24 ` Ivan Zakharyaschev
2020-02-11 17:05 ` Ivan Zakharyaschev
2020-02-11 18:14 ` Dmitry V. Levin
2020-02-12 2:14 ` Ivan Zakharyaschev
2020-02-12 8:47 ` Dmitry V. Levin
ALT Linux Team development discussions
This inbox may be cloned and mirrored by anyone:
git clone --mirror http://lore.altlinux.org/devel/0 devel/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 devel devel/ http://lore.altlinux.org/devel \
devel@altlinux.org devel@altlinux.ru devel@lists.altlinux.org devel@lists.altlinux.ru devel@linux.iplabs.ru mandrake-russian@linuxteam.iplabs.ru sisyphus@linuxteam.iplabs.ru
public-inbox-index devel
Example config snippet for mirrors.
Newsgroup available over NNTP:
nntp://lore.altlinux.org/org.altlinux.lists.devel
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git