Skip to content

Commit

Permalink
Fix a dependency cycle in DefaultStorageKeyAllocator.h (#21794)
Browse files Browse the repository at this point in the history
* Fix a dependency cycle in DefaultStorageKeyAllocator.h

When using SDK with a build system that disallows build graph
dependencies and enforces explicit dependencies being always included,
the DefaultStorageKeyAllocator's dependency on IM-layer headers
and absence from BUILD.gn mean that a cycle that could have been
detected by GN is not detected in Matter SDK, but detected by
the build.

This PR:
- Adds DefaultStorageKeyAllocator.h to the correct dependency graph
- Removes a cycle from `app/ConcreteAttributePath.h`
- Fixes-up DefaultStorageKeyAllocator to work after the cycle is broken

Testing done:
- Cert tests pass
- Unit tests pass

* Restyle
  • Loading branch information
tcarmelveilleux authored Aug 15, 2022
1 parent 38f3ebc commit 6322b8d
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 6 deletions.
6 changes: 4 additions & 2 deletions src/app/DefaultAttributePersistenceProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttribu
{
return CHIP_ERROR_BUFFER_TOO_SMALL;
}
return mStorage->SyncSetKeyValue(key.AttributeValue(aPath), aValue.data(), static_cast<uint16_t>(aValue.size()));
return mStorage->SyncSetKeyValue(key.AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue.data(),
static_cast<uint16_t>(aValue.size()));
}

CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
Expand All @@ -45,7 +46,8 @@ CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttribut

DefaultStorageKeyAllocator key;
uint16_t size = static_cast<uint16_t>(min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
ReturnErrorOnFailure(mStorage->SyncGetKeyValue(key.AttributeValue(aPath), aValue.data(), size));
ReturnErrorOnFailure(mStorage->SyncGetKeyValue(key.AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
aValue.data(), size));
EmberAfAttributeType type = aMetadata->attributeType;
if (emberAfIsStringAttributeType(type))
{
Expand Down
1 change: 1 addition & 0 deletions src/lib/support/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ static_library("support") {
"CHIPPlatformMemory.h",
"CodeUtils.h",
"DLLUtil.h",
"DefaultStorageKeyAllocator.h",
"Defer.h",
"ErrorStr.cpp",
"ErrorStr.h",
Expand Down
10 changes: 6 additions & 4 deletions src/lib/support/DefaultStorageKeyAllocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
*/
#pragma once

#include <app/ConcreteAttributePath.h>
#include <app/util/basic-types.h>
#include <lib/core/CHIPPersistentStorageDelegate.h>
#include <lib/core/DataModelTypes.h>
#include <lib/core/GroupId.h>
#include <lib/support/EnforceFormat.h>
#include <lib/support/logging/Constants.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

namespace chip {
Expand Down Expand Up @@ -93,11 +95,11 @@ class DefaultStorageKeyAllocator
}
const char * FabricKeyset(chip::FabricIndex fabric, uint16_t keyset) { return Format("f/%x/k/%x", fabric, keyset); }

const char * AttributeValue(const app::ConcreteAttributePath & aPath)
const char * AttributeValue(EndpointId endpointId, ClusterId clusterId, AttributeId attributeId)
{
// Needs at most 26 chars: 6 for "g/a///", 4 for the endpoint id, 8 each
// for the cluster and attribute ids.
return Format("g/a/%x/%" PRIx32 "/%" PRIx32, aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId);
return Format("g/a/%x/%" PRIx32 "/%" PRIx32, endpointId, clusterId, attributeId);
}

// TODO: Should store fabric-specific parts of the binding list under keys
Expand Down

0 comments on commit 6322b8d

Please sign in to comment.