[devel] [SCM] packages/apt: heads/rework-dynamic-mmap
Ivan Zakharyaschev
imz на altlinux.org
Вт Фев 11 16:47:41 MSK 2020
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
Подробная информация о списке рассылки Devel