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