Update MetadataList logic for better error handling (#37334)

* Update with review comments from 37127

* Addressing more comments

* One more comment update

* Restyled by clang-format

* Update src/app/data-model-provider/ProviderMetadataTree.h

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Author: 4 people

src/app/data-model-provider/MetadataList.cpp

@@ -82,10 +82,11 @@ CHIP_ERROR GenericAppendOnlyBuffer::EnsureAppendCapacity(size_t numElements)
 
     if (mBuffer == nullptr)
     {
-        mBuffer            = static_cast<uint8_t *>(Platform::MemoryCalloc(numElements, mElementSize));
+        mBuffer = static_cast<uint8_t *>(Platform::MemoryCalloc(numElements, mElementSize));
+        VerifyOrReturnError(mBuffer != nullptr, CHIP_ERROR_NO_MEMORY);
         mCapacity          = numElements;
         mBufferIsAllocated = true;
-        return mBuffer != nullptr ? CHIP_NO_ERROR : CHIP_ERROR_NO_MEMORY;
+        return CHIP_NO_ERROR;
     }
 
     // we already have the data in buffer. we have two choices:
@@ -100,9 +101,9 @@ CHIP_ERROR GenericAppendOnlyBuffer::EnsureAppendCapacity(size_t numElements)
     else
     {
         // this is NOT an allocated buffer, but it should become one
-        auto new_buffer    = static_cast<uint8_t *>(Platform::MemoryCalloc(mElementCount + numElements, mElementSize));
-        mBufferIsAllocated = true;
+        auto new_buffer = static_cast<uint8_t *>(Platform::MemoryCalloc(mElementCount + numElements, mElementSize));
         VerifyOrReturnError(new_buffer != nullptr, CHIP_ERROR_NO_MEMORY);
+        mBufferIsAllocated = true;
         memcpy(new_buffer, mBuffer, mElementCount * mElementSize);
         mBuffer = new_buffer;
     }
@@ -119,7 +120,7 @@ CHIP_ERROR GenericAppendOnlyBuffer::AppendSingleElementRaw(const void * buffer)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR GenericAppendOnlyBuffer::AppendElementArrayRaw(const void * buffer, size_t numElements)
+CHIP_ERROR GenericAppendOnlyBuffer::AppendElementArrayRaw(const void * __restrict__ buffer, size_t numElements)
 {
     ReturnErrorOnFailure(EnsureAppendCapacity(numElements));
 

src/app/data-model-provider/MetadataList.h

@@ -47,7 +47,7 @@ class GenericAppendOnlyBuffer
     /// Ensure that at least the specified number of elements
     /// can be appended to the internal buffer;
     ///
-    /// This will cause the internal buffer to become and allocated buffer
+    /// This will cause the internal buffer to become an allocated buffer
     CHIP_ERROR EnsureAppendCapacity(size_t numElements);
 
     bool IsEmpty() const { return mElementCount == 0; }
@@ -66,7 +66,10 @@ class GenericAppendOnlyBuffer
     ///
     /// This ALWAYS COPIES the elements internally.
     /// Additional capacity is AUTOMATICALLY ADDED.
-    CHIP_ERROR AppendElementArrayRaw(const void * buffer, size_t numElements);
+    ///
+    /// buffer MUST NOT point inside "own" buffer as mBuffer may be reallocated
+    /// as part of the appending.
+    CHIP_ERROR AppendElementArrayRaw(const void * __restrict__ buffer, size_t numElements);
 
     /// Appends a list of elements from a raw array.
     ///
@@ -93,7 +96,11 @@ class GenericAppendOnlyBuffer
 
 /// Represents a RAII instance owning a buffer.
 ///
-/// It auto-frees the owned buffer on destruction
+/// It auto-frees the owned buffer on destruction via `Platform::MemoryFree`.
+///
+/// This class is designed to be a storage class for `GenericAppendOnlyBuffer` and
+/// its subclasses (i.e. GenericAppendOnlyBuffer uses PlatformMemory and this class
+/// does the same. They MUST be kept in sync.)
 class ScopedBuffer
 {
 public:
@@ -165,6 +172,10 @@ class ListBuilder : public detail::GenericAppendOnlyBuffer
     ///
     /// Automatically attempts to allocate sufficient space to fulfill the element
     /// requirements.
+    ///
+    /// `span` MUST NOT point inside "own" buffer (and generally will not
+    /// as this class does not expose buffer access except by releasing ownership
+    /// via `Take`)
     CHIP_ERROR AppendElements(SpanType span) { return AppendElementArrayRaw(span.data(), span.size()); }
 
     /// Append a single element.

src/app/data-model-provider/ProviderMetadataTree.h

@@ -83,7 +83,11 @@ class ProviderMetadataTree
     virtual void Temporary_ReportAttributeChanged(const AttributePathParams & path) = 0;
 
     // "convenience" functions that just return the data and ignore the error
-    // This returns the builder as-is even after the error (e.g. not found would return empty data)
+    // This returns the `ListBuilder<..>::TakeBuffer` from their equivalent fuctions as-is,
+    // even after an error (e.g. not found would return empty data).
+    //
+    // Usage of these indicates no error handling (not even logging) and code should
+    // consider handling errors instead.
     ReadOnlyBuffer<EndpointEntry> EndpointsIgnoreError();
     ReadOnlyBuffer<ServerClusterEntry> ServerClustersIgnoreError(EndpointId endpointId);
     ReadOnlyBuffer<AttributeEntry> AttributesIgnoreError(const ConcreteClusterPath & path);