From e8ef1a21b6536001973022cf66d1431fcfcc9c4f Mon Sep 17 00:00:00 2001 From: Jan Date: Thu, 21 Sep 2017 10:26:59 +0200 Subject: [PATCH] fix some issues found by coverity code scan (#3256) --- arangod/Cache/BucketState.cpp | 6 +++--- arangod/Cache/BucketState.h | 2 +- arangod/Cache/Cache.cpp | 4 ++-- arangod/Cache/Cache.h | 2 +- arangod/Cache/Manager.cpp | 4 ++-- arangod/Cache/PlainBucket.cpp | 2 +- arangod/Cache/PlainBucket.h | 2 +- arangod/Cache/PlainCache.cpp | 20 +++++++++----------- arangod/Cache/PlainCache.h | 6 +++--- arangod/Cache/Table.cpp | 6 +++--- arangod/Cache/Table.h | 8 ++++---- arangod/Cache/TransactionalBucket.cpp | 2 +- arangod/Cache/TransactionalBucket.h | 2 +- arangod/Cache/TransactionalCache.cpp | 20 +++++++++----------- arangod/Cache/TransactionalCache.h | 6 +++--- arangod/Graph/EdgeDocumentToken.h | 4 ++++ lib/Basics/MutexUnlocker.h | 2 +- lib/Basics/tri-strings.cpp | 4 ---- 18 files changed, 49 insertions(+), 53 deletions(-) diff --git a/arangod/Cache/BucketState.cpp b/arangod/Cache/BucketState.cpp index a825cc7229..7011fb1f1d 100644 --- a/arangod/Cache/BucketState.cpp +++ b/arangod/Cache/BucketState.cpp @@ -48,9 +48,9 @@ bool BucketState::isLocked() const { return ((_state.load() & static_cast(Flag::locked)) > 0); } -bool BucketState::lock(int64_t maxTries, BucketState::CallbackType cb) { - int64_t attempt = 0; - while (maxTries < 0 || attempt < maxTries) { +bool BucketState::lock(uint64_t maxTries, BucketState::CallbackType cb) { + uint64_t attempt = 0; + while (attempt < maxTries) { // expect unlocked, but need to preserve migrating status uint32_t current = _state.load(std::memory_order_relaxed); uint32_t expected = current & (~static_cast(Flag::locked)); diff --git a/arangod/Cache/BucketState.h b/arangod/Cache/BucketState.h index 1fdb0ebe26..2c9428c636 100644 --- a/arangod/Cache/BucketState.h +++ b/arangod/Cache/BucketState.h @@ -97,7 +97,7 @@ struct BucketState { /// locked or not. The optional second parameter is a function which will be /// called upon successfully locking the state. ////////////////////////////////////////////////////////////////////////////// - bool lock(int64_t maxTries = -1LL, BucketState::CallbackType cb = []() -> void {}); + bool lock(uint64_t maxTries = UINT64_MAX, BucketState::CallbackType cb = []() -> void {}); ////////////////////////////////////////////////////////////////////////////// /// @brief Unlocks the state. Requires state to be locked. diff --git a/arangod/Cache/Cache.cpp b/arangod/Cache/Cache.cpp index cab053a508..a581ff5d7d 100644 --- a/arangod/Cache/Cache.cpp +++ b/arangod/Cache/Cache.cpp @@ -48,7 +48,7 @@ uint64_t Cache::_findStatsCapacity = 16384; Cache::ConstructionGuard::ConstructionGuard() {} -Cache::Cache(ConstructionGuard guard, Manager* manager, uint64_t id, Metadata metadata, +Cache::Cache(ConstructionGuard guard, Manager* manager, uint64_t id, Metadata&& metadata, std::shared_ptr table, bool enableWindowedStats, std::function bucketClearer, size_t slotsPerBucket) @@ -60,7 +60,7 @@ Cache::Cache(ConstructionGuard guard, Manager* manager, uint64_t id, Metadata me _findMisses(), _manager(manager), _id(id), - _metadata(metadata), + _metadata(std::move(metadata)), _tableShrdPtr(table), _table(table.get()), _bucketClearer(bucketClearer(&_metadata)), diff --git a/arangod/Cache/Cache.h b/arangod/Cache/Cache.h index 3efb33c31a..8bb16f2029 100644 --- a/arangod/Cache/Cache.h +++ b/arangod/Cache/Cache.h @@ -72,7 +72,7 @@ class Cache : public std::enable_shared_from_this { static const uint64_t minLogSize; public: - Cache(ConstructionGuard guard, Manager* manager, uint64_t id, Metadata metadata, + Cache(ConstructionGuard guard, Manager* manager, uint64_t id, Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats, std::function bucketClearer, size_t slotsPerBucket); diff --git a/arangod/Cache/Manager.cpp b/arangod/Cache/Manager.cpp index 3a11d5bc42..3a6925fcdd 100644 --- a/arangod/Cache/Manager.cpp +++ b/arangod/Cache/Manager.cpp @@ -133,11 +133,11 @@ std::shared_ptr Manager::createCache(CacheType type, if (allowed) { switch (type) { case CacheType::Plain: - result = PlainCache::create(this, id, metadata, table, + result = PlainCache::create(this, id, std::move(metadata), table, enableWindowedStats); break; case CacheType::Transactional: - result = TransactionalCache::create(this, id, metadata, table, + result = TransactionalCache::create(this, id, std::move(metadata), table, enableWindowedStats); break; default: diff --git a/arangod/Cache/PlainBucket.cpp b/arangod/Cache/PlainBucket.cpp index 1b3f99db8c..6d1ccf34b7 100644 --- a/arangod/Cache/PlainBucket.cpp +++ b/arangod/Cache/PlainBucket.cpp @@ -35,7 +35,7 @@ PlainBucket::PlainBucket() { clear(); } -bool PlainBucket::lock(int64_t maxTries) { return _state.lock(maxTries); } +bool PlainBucket::lock(uint64_t maxTries) { return _state.lock(maxTries); } void PlainBucket::unlock() { TRI_ASSERT(_state.isLocked()); diff --git a/arangod/Cache/PlainBucket.h b/arangod/Cache/PlainBucket.h index 5c169238c6..46ded43b92 100644 --- a/arangod/Cache/PlainBucket.h +++ b/arangod/Cache/PlainBucket.h @@ -67,7 +67,7 @@ struct PlainBucket { ////////////////////////////////////////////////////////////////////////////// /// @brief Attempt to lock bucket (failing after maxTries attempts). ////////////////////////////////////////////////////////////////////////////// - bool lock(int64_t maxTries); + bool lock(uint64_t maxTries); ////////////////////////////////////////////////////////////////////////////// /// @brief Unlock the bucket. Requires bucket to be locked. diff --git a/arangod/Cache/PlainCache.cpp b/arangod/Cache/PlainCache.cpp index 0165d454fc..d6cef43cad 100644 --- a/arangod/Cache/PlainCache.cpp +++ b/arangod/Cache/PlainCache.cpp @@ -175,17 +175,17 @@ uint64_t PlainCache::allocationSize(bool enableWindowedStats) { : 0); } -std::shared_ptr PlainCache::create(Manager* manager, uint64_t id, Metadata metadata, +std::shared_ptr PlainCache::create(Manager* manager, uint64_t id, Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats) { return std::make_shared(Cache::ConstructionGuard(), manager, id, - metadata, table, enableWindowedStats); + std::move(metadata), table, enableWindowedStats); } PlainCache::PlainCache(Cache::ConstructionGuard guard, Manager* manager, uint64_t id, - Metadata metadata, std::shared_ptr
table, + Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats) - : Cache(guard, manager, id, metadata, table, enableWindowedStats, + : Cache(guard, manager, id, std::move(metadata), table, enableWindowedStats, PlainCache::bucketClearer, PlainBucket::slotsData) {} PlainCache::~PlainCache() { @@ -233,10 +233,9 @@ void PlainCache::migrateBucket(void* sourcePtr, source->lock(Cache::triesGuarantee); // lock target bucket(s) - int64_t tries = Cache::triesGuarantee; - targets->applyToAllBuckets([tries](void* ptr) -> bool { + targets->applyToAllBuckets([](void* ptr) -> bool { auto targetBucket = reinterpret_cast(ptr); - return targetBucket->lock(tries); + return targetBucket->lock(Cache::triesGuarantee); }); for (size_t j = 0; j < PlainBucket::slotsData; j++) { @@ -287,7 +286,7 @@ void PlainCache::migrateBucket(void* sourcePtr, } std::tuple PlainCache::getBucket( - uint32_t hash, int64_t maxTries, bool singleOperation) { + uint32_t hash, uint64_t maxTries, bool singleOperation) { Result status; PlainBucket* bucket = nullptr; Table* source = nullptr; @@ -315,10 +314,9 @@ std::tuple PlainCache::getBucket( } Table::BucketClearer PlainCache::bucketClearer(Metadata* metadata) { - int64_t tries = Cache::triesGuarantee; - return [metadata, tries](void* ptr) -> void { + return [metadata](void* ptr) -> void { auto bucket = reinterpret_cast(ptr); - bucket->lock(tries); + bucket->lock(Cache::triesGuarantee); for (size_t j = 0; j < PlainBucket::slotsData; j++) { if (bucket->_cachedData[j] != nullptr) { uint64_t size = bucket->_cachedData[j]->size(); diff --git a/arangod/Cache/PlainCache.h b/arangod/Cache/PlainCache.h index 8286d37cc3..79fea0e940 100644 --- a/arangod/Cache/PlainCache.h +++ b/arangod/Cache/PlainCache.h @@ -54,7 +54,7 @@ namespace cache { class PlainCache final : public Cache { public: PlainCache(Cache::ConstructionGuard guard, Manager* manager, uint64_t id, - Metadata metadata, std::shared_ptr
table, + Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats); ~PlainCache(); @@ -105,7 +105,7 @@ class PlainCache final : public Cache { private: static uint64_t allocationSize(bool enableWindowedStats); - static std::shared_ptr create(Manager* manager, uint64_t id, Metadata metadata, + static std::shared_ptr create(Manager* manager, uint64_t id, Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats); @@ -116,7 +116,7 @@ class PlainCache final : public Cache { // helpers std::tuple getBucket( - uint32_t hash, int64_t maxTries, bool singleOperation = true); + uint32_t hash, uint64_t maxTries, bool singleOperation = true); uint32_t getIndex(uint32_t hash, bool useAuxiliary) const; static Table::BucketClearer bucketClearer(Metadata* metadata); diff --git a/arangod/Cache/Table.cpp b/arangod/Cache/Table.cpp index 371039f945..b25defecc4 100644 --- a/arangod/Cache/Table.cpp +++ b/arangod/Cache/Table.cpp @@ -34,7 +34,7 @@ using namespace arangodb::cache; const uint32_t Table::minLogSize = 8; const uint32_t Table::maxLogSize = 32; -bool Table::GenericBucket::lock(int64_t maxTries) { +bool Table::GenericBucket::lock(uint64_t maxTries) { return _state.lock(maxTries); } @@ -103,7 +103,7 @@ uint64_t Table::size() const { return _size; } uint32_t Table::logSize() const { return _logSize; } std::pair Table::fetchAndLockBucket( - uint32_t hash, int64_t maxTries) { + uint32_t hash, uint64_t maxTries) { GenericBucket* bucket = nullptr; Table* source = nullptr; bool ok = _lock.readLock(maxTries); @@ -217,7 +217,7 @@ void Table::enable() { _lock.writeUnlock(); } -bool Table::isEnabled(int64_t maxTries) { +bool Table::isEnabled(uint64_t maxTries) { bool ok = _lock.readLock(maxTries); if (ok) { ok = !_disabled; diff --git a/arangod/Cache/Table.h b/arangod/Cache/Table.h index 3ce4beaf9f..f3328a9467 100644 --- a/arangod/Cache/Table.h +++ b/arangod/Cache/Table.h @@ -45,7 +45,7 @@ class Table : public std::enable_shared_from_this
{ static const uint32_t minLogSize; static const uint32_t maxLogSize; static constexpr uint32_t standardLogSizeAdjustment = 6; - static constexpr int64_t triesGuarantee = -1; + static constexpr uint64_t triesGuarantee = UINT64_MAX; static constexpr uint64_t padding = BUCKET_SIZE; typedef std::function BucketClearer; @@ -54,7 +54,7 @@ class Table : public std::enable_shared_from_this
{ struct GenericBucket { BucketState _state; uint8_t _filler[BUCKET_SIZE - sizeof(BucketState)]; - bool lock(int64_t maxTries); + bool lock(uint64_t maxTries); void unlock(); bool isMigrated() const; }; @@ -119,7 +119,7 @@ class Table : public std::enable_shared_from_this
{ /// table for the bucket returned as the first member. ////////////////////////////////////////////////////////////////////////////// std::pair fetchAndLockBucket( - uint32_t hash, int64_t maxTries = -1); + uint32_t hash, uint64_t maxTries = UINT64_MAX); ////////////////////////////////////////////////////////////////////////////// /// @brief Sets the auxiliary table. @@ -209,7 +209,7 @@ class Table : public std::enable_shared_from_this
{ private: void disable(); - bool isEnabled(int64_t maxTries = triesGuarantee); + bool isEnabled(uint64_t maxTries = triesGuarantee); static void defaultClearer(void* ptr); }; diff --git a/arangod/Cache/TransactionalBucket.cpp b/arangod/Cache/TransactionalBucket.cpp index 7e632fe95f..423a2c4b76 100644 --- a/arangod/Cache/TransactionalBucket.cpp +++ b/arangod/Cache/TransactionalBucket.cpp @@ -35,7 +35,7 @@ TransactionalBucket::TransactionalBucket() { clear(); } -bool TransactionalBucket::lock(int64_t maxTries) { +bool TransactionalBucket::lock(uint64_t maxTries) { return _state.lock(maxTries); } diff --git a/arangod/Cache/TransactionalBucket.h b/arangod/Cache/TransactionalBucket.h index ad5401dd5f..6cdb6e5119 100644 --- a/arangod/Cache/TransactionalBucket.h +++ b/arangod/Cache/TransactionalBucket.h @@ -73,7 +73,7 @@ struct TransactionalBucket { ////////////////////////////////////////////////////////////////////////////// /// @brief Attempt to lock bucket (failing after maxTries attempts). ////////////////////////////////////////////////////////////////////////////// - bool lock(int64_t maxTries); + bool lock(uint64_t maxTries); ////////////////////////////////////////////////////////////////////////////// /// @brief Unlock the bucket. Requires bucket to be locked. diff --git a/arangod/Cache/TransactionalCache.cpp b/arangod/Cache/TransactionalCache.cpp index 4baaa2af5b..fa37583f99 100644 --- a/arangod/Cache/TransactionalCache.cpp +++ b/arangod/Cache/TransactionalCache.cpp @@ -213,19 +213,19 @@ uint64_t TransactionalCache::allocationSize(bool enableWindowedStats) { std::shared_ptr TransactionalCache::create(Manager* manager, uint64_t id, - Metadata metadata, + Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats) { return std::make_shared(Cache::ConstructionGuard(), - manager, id, metadata, table, + manager, id, std::move(metadata), table, enableWindowedStats); } TransactionalCache::TransactionalCache(Cache::ConstructionGuard guard, - Manager* manager, uint64_t id, Metadata metadata, + Manager* manager, uint64_t id, Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats) - : Cache(guard, manager, id, metadata, table, enableWindowedStats, + : Cache(guard, manager, id, std::move(metadata), table, enableWindowedStats, TransactionalCache::bucketClearer, TransactionalBucket::slotsData) { } @@ -277,10 +277,9 @@ void TransactionalCache::migrateBucket(void* sourcePtr, term = std::max(term, source->_blacklistTerm); // lock target bucket(s) - int64_t tries = Cache::triesGuarantee; - targets->applyToAllBuckets([&term, tries](void* ptr) -> bool { + targets->applyToAllBuckets([&term](void* ptr) -> bool { auto targetBucket = reinterpret_cast(ptr); - bool locked = targetBucket->lock(tries); + bool locked = targetBucket->lock(Cache::triesGuarantee); term = std::max(term, targetBucket->_blacklistTerm); return locked; }); @@ -374,7 +373,7 @@ void TransactionalCache::migrateBucket(void* sourcePtr, } std::tuple -TransactionalCache::getBucket(uint32_t hash, int64_t maxTries, +TransactionalCache::getBucket(uint32_t hash, uint64_t maxTries, bool singleOperation) { Result status; TransactionalBucket* bucket = nullptr; @@ -405,10 +404,9 @@ TransactionalCache::getBucket(uint32_t hash, int64_t maxTries, } Table::BucketClearer TransactionalCache::bucketClearer(Metadata* metadata) { - int64_t tries = Cache::triesGuarantee; - return [metadata, tries](void* ptr) -> void { + return [metadata](void* ptr) -> void { auto bucket = reinterpret_cast(ptr); - bucket->lock(tries); + bucket->lock(Cache::triesGuarantee); for (size_t j = 0; j < TransactionalBucket::slotsData; j++) { if (bucket->_cachedData[j] != nullptr) { uint64_t size = bucket->_cachedData[j]->size(); diff --git a/arangod/Cache/TransactionalCache.h b/arangod/Cache/TransactionalCache.h index b674e16b6b..7cbaa6fe71 100644 --- a/arangod/Cache/TransactionalCache.h +++ b/arangod/Cache/TransactionalCache.h @@ -62,7 +62,7 @@ namespace cache { class TransactionalCache final : public Cache { public: TransactionalCache(Cache::ConstructionGuard guard, Manager* manager, uint64_t id, - Metadata metadata, std::shared_ptr
table, + Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats); ~TransactionalCache(); @@ -121,7 +121,7 @@ class TransactionalCache final : public Cache { private: static uint64_t allocationSize(bool enableWindowedStats); - static std::shared_ptr create(Manager* manager, uint64_t id, Metadata metadata, + static std::shared_ptr create(Manager* manager, uint64_t id, Metadata&& metadata, std::shared_ptr
table, bool enableWindowedStats); @@ -132,7 +132,7 @@ class TransactionalCache final : public Cache { // helpers std::tuple getBucket( - uint32_t hash, int64_t maxTries, bool singleOperation = true); + uint32_t hash, uint64_t maxTries, bool singleOperation = true); uint32_t getIndex(uint32_t hash, bool useAuxiliary) const; static Table::BucketClearer bucketClearer(Metadata* metadata); diff --git a/arangod/Graph/EdgeDocumentToken.h b/arangod/Graph/EdgeDocumentToken.h index 6f6bb2c9d8..2d7ad2d2c4 100644 --- a/arangod/Graph/EdgeDocumentToken.h +++ b/arangod/Graph/EdgeDocumentToken.h @@ -37,7 +37,11 @@ namespace graph { struct EdgeDocumentToken { EdgeDocumentToken() noexcept +#ifdef ARANGODB_ENABLE_MAINTAINER_MODE + : _data(0, DocumentIdentifierToken()), _type(TokenType::NONE) {} +#else : _data(0, DocumentIdentifierToken()) {} +#endif EdgeDocumentToken(EdgeDocumentToken&& edtkn) noexcept { #ifdef ARANGODB_ENABLE_MAINTAINER_MODE diff --git a/lib/Basics/MutexUnlocker.h b/lib/Basics/MutexUnlocker.h index d269b10941..19e5ce3d1c 100644 --- a/lib/Basics/MutexUnlocker.h +++ b/lib/Basics/MutexUnlocker.h @@ -75,8 +75,8 @@ class MutexUnlocker { /// @brief unlocks the mutex if we own it bool unlock() { if (_isLocked) { - _mutex->unlock(); _isLocked = false; + _mutex->unlock(); return true; } return false; diff --git a/lib/Basics/tri-strings.cpp b/lib/Basics/tri-strings.cpp index 38b061aecc..d4882d9b9f 100644 --- a/lib/Basics/tri-strings.cpp +++ b/lib/Basics/tri-strings.cpp @@ -659,10 +659,6 @@ char* TRI_SHA256String(char const* source, size_t sourceLen, size_t* dstLen) { } *dstLen = SHA256_DIGEST_LENGTH; - if (dst == nullptr) { - return nullptr; - } - SHA256((unsigned char const*)source, sourceLen, dst); return (char*)dst;