From 711a2a6eea557acfd3b0737ec428334ef40155b9 Mon Sep 17 00:00:00 2001 From: Ingo Weinhold Date: Sat, 13 Apr 2013 02:08:43 +0200 Subject: [PATCH] BPackageInfoSet: Add copy constructor, assignment operator * Implement copy-on-write support. * Add copy constructor and assignment operator. * Remove Init(). Initialize lazily instead. Since AddInfo() can fail and we check initialization anyway, there's no point in having an explicit Init(). Given that there was only one invocation of Init() in the package kit and its users, it was very likely missing in some places. * Fix a few places where we ignored that the PackageMap actually contains lists of PackageInfo objects. --- headers/os/package/PackageInfoSet.h | 12 +- src/kits/package/PackageInfoSet.cpp | 188 ++++++++++++++++++++------- src/kits/package/RepositoryCache.cpp | 6 +- 3 files changed, 148 insertions(+), 58 deletions(-) diff --git a/headers/os/package/PackageInfoSet.h b/headers/os/package/PackageInfoSet.h index f2545ff73e..35fbbdc2be 100644 --- a/headers/os/package/PackageInfoSet.h +++ b/headers/os/package/PackageInfoSet.h @@ -21,16 +21,20 @@ public: public: BPackageInfoSet(); + BPackageInfoSet(const BPackageInfoSet& other); virtual ~BPackageInfoSet(); - status_t Init(); - status_t AddInfo(const BPackageInfo& info); void MakeEmpty(); uint32 CountInfos() const; Iterator GetIterator() const; + BPackageInfoSet& operator=(const BPackageInfoSet& other); + +private: + bool _CopyOnWrite(); + private: struct PackageInfo; struct PackageInfoHashDefinition; @@ -46,7 +50,7 @@ private: class BPackageInfoSet::Iterator { public: Iterator(); - Iterator(const BPackageInfoSet* set); + Iterator(const PackageMap* map); bool HasNext() const; const BPackageInfo* Next(); @@ -55,7 +59,7 @@ private: friend class BRepositoryCache; private: - const BPackageInfoSet* fSet; + const PackageMap* fMap; PackageInfo* fNextInfo; }; diff --git a/src/kits/package/PackageInfoSet.cpp b/src/kits/package/PackageInfoSet.cpp index 315cedcd92..997e1dced5 100644 --- a/src/kits/package/PackageInfoSet.cpp +++ b/src/kits/package/PackageInfoSet.cpp @@ -12,6 +12,10 @@ #include +#include + +#include + #include #include @@ -33,6 +37,16 @@ struct BPackageInfoSet::PackageInfo : public BPackageInfo { listNext(NULL) { } + + void DeleteList() + { + PackageInfo* info = this; + while (info != NULL) { + PackageInfo* next = info->listNext; + delete info; + info = next; + } + } }; @@ -68,8 +82,8 @@ struct BPackageInfoSet::PackageInfoHashDefinition { // #pragma mark - PackageMap -struct BPackageInfoSet::PackageMap - : public BOpenHashTable { +struct BPackageInfoSet::PackageMap : public BReferenceable, + public BOpenHashTable { PackageMap() : @@ -79,12 +93,34 @@ struct BPackageInfoSet::PackageMap ~PackageMap() { - PackageInfo* info = Clear(true); - while (info != NULL) { - PackageInfo* next = info->hashNext; - delete info; - info = next; + DeleteAllPackageInfos(); + } + + static PackageMap* Create() + { + PackageMap* map = new(std::nothrow) PackageMap; + if (map == NULL || map->Init() != B_OK) { + delete map; + return NULL; } + + return map; + } + + PackageMap* Clone() const + { + PackageMap* newMap = Create(); + if (newMap == NULL) + return NULL; + ObjectDeleter newMapDeleter(newMap); + + for (BPackageInfoSet::Iterator it(this); it.HasNext();) { + const BPackageInfo* info = it.Next(); + if (newMap->AddNewPackageInfo(*info) != B_OK) + return NULL; + } + + return newMapDeleter.Detach(); } void AddPackageInfo(PackageInfo* info) @@ -98,6 +134,32 @@ struct BPackageInfoSet::PackageMap fCount++; } + status_t AddNewPackageInfo(const BPackageInfo& oldInfo) + { + PackageInfo* info = new(std::nothrow) PackageInfo(oldInfo); + if (info == NULL) + return B_NO_MEMORY; + ObjectDeleter infoDeleter(info); + + status_t error = info->InitCheck(); + if (error != B_OK) + return error; + + AddPackageInfo(infoDeleter.Detach()); + + return B_OK; + } + + void DeleteAllPackageInfos() + { + PackageInfo* info = Clear(true); + while (info != NULL) { + PackageInfo* next = info->hashNext; + info->DeleteList(); + info = next; + } + } + uint32 CountPackageInfos() const { return fCount; @@ -113,19 +175,20 @@ private: BPackageInfoSet::Iterator::Iterator() : - fSet(NULL), + fMap(NULL), fNextInfo(NULL) { } -BPackageInfoSet::Iterator::Iterator(const BPackageInfoSet* set) +BPackageInfoSet::Iterator::Iterator(const PackageMap* map) : - fSet(set), - fNextInfo(fSet->fPackageMap->GetIterator().Next()) + fMap(map), + fNextInfo(map->GetIterator().Next()) { } + bool BPackageInfoSet::Iterator::HasNext() const { @@ -145,7 +208,7 @@ BPackageInfoSet::Iterator::Next() } else { // get next in hash table PackageMap::Iterator iterator - = fSet->fPackageMap->GetIterator(fNextInfo->Name()); + = fMap->GetIterator(fNextInfo->Name()); iterator.Next(); fNextInfo = iterator.Next(); } @@ -160,64 +223,52 @@ BPackageInfoSet::Iterator::Next() BPackageInfoSet::BPackageInfoSet() : - fPackageMap(new(std::nothrow) PackageMap) + fPackageMap(NULL) { } BPackageInfoSet::~BPackageInfoSet() { - MakeEmpty(); - delete fPackageMap; + if (fPackageMap != NULL) + fPackageMap->ReleaseReference(); +} + + +BPackageInfoSet::BPackageInfoSet(const BPackageInfoSet& other) + : + fPackageMap(other.fPackageMap) +{ + if (fPackageMap != NULL) + fPackageMap->AcquireReference(); } status_t -BPackageInfoSet::Init() +BPackageInfoSet::AddInfo(const BPackageInfo& info) { - return fPackageMap->Init(); -} - - -status_t -BPackageInfoSet::AddInfo(const BPackageInfo& _info) -{ - if (fPackageMap == NULL) - return B_NO_INIT; - - PackageInfo* info = new(std::nothrow) PackageInfo(_info); - if (info == NULL) + if (!_CopyOnWrite()) return B_NO_MEMORY; - status_t error = info->InitCheck(); - if (error != B_OK) { - delete info; - return error; - } - - if (PackageInfo* oldInfo = fPackageMap->Lookup(info->Name())) { - // TODO: Check duplicates? - info->listNext = oldInfo->listNext; - oldInfo->listNext = info; - } else - fPackageMap->Insert(info); - - return B_OK; + return fPackageMap->AddNewPackageInfo(info); } void BPackageInfoSet::MakeEmpty() { - if (fPackageMap == NULL) + if (fPackageMap == NULL || fPackageMap->CountPackageInfos() == 0) return; - PackageInfo* info = fPackageMap->Clear(true); - while (info != NULL) { - PackageInfo* next = info->hashNext; - delete info; - info = next; + // If our map is shared, just set it to NULL. + if (fPackageMap->CountReferences() != 1) { + fPackageMap->ReleaseReference(); + fPackageMap = NULL; + return; } + + // Our map is not shared -- make it empty. + fPackageMap->DeleteAllPackageInfos(); } @@ -234,7 +285,46 @@ BPackageInfoSet::CountInfos() const BPackageInfoSet::Iterator BPackageInfoSet::GetIterator() const { - return Iterator(this); + return Iterator(fPackageMap); +} + + +BPackageInfoSet& +BPackageInfoSet::operator=(const BPackageInfoSet& other) +{ + if (other.fPackageMap == fPackageMap) + return *this; + + if (fPackageMap != NULL) + fPackageMap->ReleaseReference(); + + fPackageMap = other.fPackageMap; + + if (fPackageMap != NULL) + fPackageMap->AcquireReference(); + + return *this; +} + + +bool +BPackageInfoSet::_CopyOnWrite() +{ + if (fPackageMap == NULL) { + fPackageMap = PackageMap::Create(); + return fPackageMap != NULL; + } + + if (fPackageMap->CountReferences() == 1) + return true; + + PackageMap* newMap = fPackageMap->Clone(); + if (newMap == NULL) + return false; + + fPackageMap->ReleaseReference(); + fPackageMap = newMap; + return true; } diff --git a/src/kits/package/RepositoryCache.cpp b/src/kits/package/RepositoryCache.cpp index d6f5b2759b..c62c204997 100644 --- a/src/kits/package/RepositoryCache.cpp +++ b/src/kits/package/RepositoryCache.cpp @@ -243,15 +243,11 @@ BRepositoryCache::SetTo(const BEntry& entry) fPackages.MakeEmpty(); fEntry.Unset(); - // init package info set - status_t result = fPackages.Init(); - if (result != B_OK) - return result; - // get cache file path fEntry = entry; BPath repositoryCachePath; + status_t result; if ((result = entry.GetPath(&repositoryCachePath)) != B_OK) return result;