Skip to content

Commit

Permalink
Add a helper function to allow ErrorStr not using static char array (p…
Browse files Browse the repository at this point in the history 
…roject-chip#36391)

* Add a helper function to allow ErrorStr does not rely on static char
array

* Use dedicated storage for ErrorStr and add unit tests

* Restyled by whitespace

* Restyled by clang-format

* remove size()

---------

Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@yyzhong-g @restyled-commits
yyzhong-g and restyled-commits authored Nov 8, 2024
1 parent 585a74c commit 5ace630
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 48 deletions.
43 changes: 27 additions & 16 deletions src/lib/core/ErrorStr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace chip {
/**
* Static buffer to store the formatted error string.
*/
static char sErrorStr[CHIP_CONFIG_ERROR_STR_SIZE];
static ErrorStrStorage sErrorStr;

/**
* Linked-list of error formatter functions.
Expand All @@ -38,7 +38,7 @@ static ErrorFormatter * sErrorFormatterList = nullptr;

/**
* This routine returns a human-readable NULL-terminated C string
* describing the provided error.
* describing the provided error. This uses the global static storage.
*
* @param[in] err The error for format and describe.
* @param[in] withSourceLocation Whether or not to include the source
Expand All @@ -50,8 +50,26 @@ static ErrorFormatter * sErrorFormatterList = nullptr;
*/
DLL_EXPORT const char * ErrorStr(CHIP_ERROR err, bool withSourceLocation)
{
char * formattedError = sErrorStr;
uint16_t formattedSpace = sizeof(sErrorStr);
return ErrorStr(err, withSourceLocation, sErrorStr);
}

/**
* This routine writess a human-readable NULL-terminated C string into the buf
* which describes the provided error.
*
* @param[in] err The error for format and describe.
* @param[in] withSourceLocation Whether or not to include the source
* @param[in] storage ErrorStrStorage to write into
* location in the output string. Only used if CHIP_CONFIG_ERROR_SOURCE &&
* !CHIP_CONFIG_SHORT_ERROR_STR. Defaults to true.
*
* @return A pointer to a NULL-terminated C string describing the
* provided error.
*/
DLL_EXPORT const char * ErrorStr(CHIP_ERROR err, bool withSourceLocation, ErrorStrStorage & storage)
{
char * formattedError = storage.buff;
uint16_t formattedSpace = storage.kBufferSize;

#if CHIP_CONFIG_ERROR_SOURCE && !CHIP_CONFIG_SHORT_ERROR_STR

Expand All @@ -65,34 +83,27 @@ DLL_EXPORT const char * ErrorStr(CHIP_ERROR err, bool withSourceLocation)
formattedError += n;
formattedSpace = static_cast<uint16_t>(formattedSpace - n);
}
if (err == CHIP_NO_ERROR)
{
(void) snprintf(formattedError, formattedSpace, CHIP_NO_ERROR_STRING);
return sErrorStr;
}

#else // CHIP_CONFIG_ERROR_SOURCE && !CHIP_CONFIG_SHORT_ERROR_STR
#endif // CHIP_CONFIG_ERROR_SOURCE && !CHIP_CONFIG_SHORT_ERROR_STR

if (err == CHIP_NO_ERROR)
{
return CHIP_NO_ERROR_STRING;
(void) snprintf(formattedError, formattedSpace, CHIP_NO_ERROR_STRING);
return storage.buff;
}

#endif // CHIP_CONFIG_ERROR_SOURCE && !CHIP_CONFIG_SHORT_ERROR_STR

// Search the registered error formatter for one that will format the given
// error code.
for (const ErrorFormatter * errFormatter = sErrorFormatterList; errFormatter != nullptr; errFormatter = errFormatter->Next)
{
if (errFormatter->FormatError(formattedError, formattedSpace, err))
{
return sErrorStr;
return storage.buff;
}
}

// Use a default formatting if no formatter found.
FormatError(formattedError, formattedSpace, nullptr, err, nullptr);
return sErrorStr;
return storage.buff;
}

/**
Expand Down
7 changes: 7 additions & 0 deletions src/lib/core/ErrorStr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,14 @@ struct ErrorFormatter
ErrorFormatter * Next;
};

struct ErrorStrStorage
{
char buff[CHIP_CONFIG_ERROR_STR_SIZE];
constexpr static uint16_t kBufferSize = sizeof(buff);
};

extern const char * ErrorStr(CHIP_ERROR err, bool withSourceLocation = true);
extern const char * ErrorStr(CHIP_ERROR err, bool withSourceLocation, ErrorStrStorage & storage);
extern void RegisterErrorFormatter(ErrorFormatter * errFormatter);
extern void DeregisterErrorFormatter(ErrorFormatter * errFormatter);
extern void FormatError(char * buf, uint16_t bufSize, const char * subsys, CHIP_ERROR err, const char * desc);
Expand Down
104 changes: 72 additions & 32 deletions src/lib/core/tests/TestCHIPErrorStr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,31 @@ static const CHIP_ERROR kTestElements[] =
};
// clang-format on

void CheckCoreErrorStrHelper(const char * errStr, CHIP_ERROR err)
{
char expectedText[9];

// Assert that the error string contains the error number in hex.
snprintf(expectedText, sizeof(expectedText), "%08" PRIX32, static_cast<uint32_t>(err.AsInteger()));
EXPECT_TRUE((strstr(errStr, expectedText) != nullptr));

#if !CHIP_CONFIG_SHORT_ERROR_STR
// Assert that the error string contains a description, which is signaled
// by a presence of a colon proceeding the description.
EXPECT_TRUE((strchr(errStr, ':') != nullptr));
#endif // !CHIP_CONFIG_SHORT_ERROR_STR

#if CHIP_CONFIG_ERROR_SOURCE
// GetFile() should be relative to ${chip_root}
char const * const file = err.GetFile();
ASSERT_NE(file, nullptr);
EXPECT_EQ(strstr(file, "src/lib/core/"), file);

// File should be included in the error.
EXPECT_NE(strstr(errStr, file), nullptr);
#endif // CHIP_CONFIG_ERROR_SOURCE
}

TEST(TestCHIPErrorStr, CheckCoreErrorStr)
{
// Register the layer error formatter
Expand All @@ -179,29 +204,46 @@ TEST(TestCHIPErrorStr, CheckCoreErrorStr)
// For each defined error...
for (const auto & err : kTestElements)
{
const char * errStr = ErrorStr(err);
char expectedText[9];
// ErrorStr with static char array.
CheckCoreErrorStrHelper(ErrorStr(err, /*withSourceLocation=*/true), err);
}
}

TEST(TestCHIPErrorStr, CheckCoreErrorStrStorage)
{
// Register the layer error formatter

RegisterCHIPLayerErrorFormatter();

// For each defined error...
for (const auto & err : kTestElements)
{
// ErrorStr with given storage.
ErrorStrStorage storage;
CheckCoreErrorStrHelper(ErrorStr(err, /*withSourceLocation=*/true, storage), err);
}
}

void CheckCoreErrorStrWithoutSourceLocationHelper(const char * errStr, CHIP_ERROR err)
{
char expectedText[9];

// Assert that the error string contains the error number in hex.
snprintf(expectedText, sizeof(expectedText), "%08" PRIX32, static_cast<uint32_t>(err.AsInteger()));
EXPECT_TRUE((strstr(errStr, expectedText) != nullptr));
// Assert that the error string contains the error number in hex.
snprintf(expectedText, sizeof(expectedText), "%08" PRIX32, static_cast<uint32_t>(err.AsInteger()));
EXPECT_TRUE((strstr(errStr, expectedText) != nullptr));

#if !CHIP_CONFIG_SHORT_ERROR_STR
// Assert that the error string contains a description, which is signaled
// by a presence of a colon proceeding the description.
EXPECT_TRUE((strchr(errStr, ':') != nullptr));
// Assert that the error string contains a description, which is signaled
// by a presence of a colon proceeding the description.
EXPECT_TRUE((strchr(errStr, ':') != nullptr));
#endif // !CHIP_CONFIG_SHORT_ERROR_STR

#if CHIP_CONFIG_ERROR_SOURCE
// GetFile() should be relative to ${chip_root}
char const * const file = err.GetFile();
ASSERT_NE(file, nullptr);
EXPECT_EQ(strstr(file, "src/lib/core/"), file);

// File should be included in the error.
EXPECT_NE(strstr(errStr, file), nullptr);
char const * const file = err.GetFile();
ASSERT_NE(file, nullptr);
// File should not be included in the error.
EXPECT_EQ(strstr(errStr, file), nullptr);
#endif // CHIP_CONFIG_ERROR_SOURCE
}
}

TEST(TestCHIPErrorStr, CheckCoreErrorStrWithoutSourceLocation)
Expand All @@ -213,24 +255,22 @@ TEST(TestCHIPErrorStr, CheckCoreErrorStrWithoutSourceLocation)
// For each defined error...
for (const auto & err : kTestElements)
{
const char * errStr = ErrorStr(err, /*withSourceLocation=*/false);
char expectedText[9];
// ErrorStr with static char array.
CheckCoreErrorStrWithoutSourceLocationHelper(ErrorStr(err, /*withSourceLocation=*/false), err);
}
}

// Assert that the error string contains the error number in hex.
snprintf(expectedText, sizeof(expectedText), "%08" PRIX32, static_cast<uint32_t>(err.AsInteger()));
EXPECT_TRUE((strstr(errStr, expectedText) != nullptr));
TEST(TestCHIPErrorStr, CheckCoreErrorStrStorageWithoutSourceLocation)
{
// Register the layer error formatter

#if !CHIP_CONFIG_SHORT_ERROR_STR
// Assert that the error string contains a description, which is signaled
// by a presence of a colon proceeding the description.
EXPECT_TRUE((strchr(errStr, ':') != nullptr));
#endif // !CHIP_CONFIG_SHORT_ERROR_STR
RegisterCHIPLayerErrorFormatter();

#if CHIP_CONFIG_ERROR_SOURCE
char const * const file = err.GetFile();
ASSERT_NE(file, nullptr);
// File should not be included in the error.
EXPECT_EQ(strstr(errStr, file), nullptr);
#endif // CHIP_CONFIG_ERROR_SOURCE
// For each defined error...
for (const auto & err : kTestElements)
{
// ErrorStr with given storage.
ErrorStrStorage storage;
CheckCoreErrorStrWithoutSourceLocationHelper(ErrorStr(err, /*withSourceLocation=*/false, storage), err);
}
}
7 changes: 7 additions & 0 deletions src/lib/support/tests/TestErrorStr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ TEST(TestErrorStr, CheckNoError)
EXPECT_STREQ(CHECK_AND_SKIP_SOURCE(ErrorStr(CHIP_NO_ERROR)), CHIP_NO_ERROR_STRING);
}

TEST(TestErrorStr, CheckErrorWithProvidedStorage)
{
ErrorStrStorage storage;
EXPECT_STREQ(CHECK_AND_SKIP_SOURCE(ErrorStr(CHIP_NO_ERROR, true, storage)), CHIP_NO_ERROR_STRING);
EXPECT_STREQ(CHECK_AND_SKIP_SOURCE(ErrorStr(CHIP_ERROR_INTERNAL, true, storage)), "Error 0x000000AC");
}

TEST(TestErrorStr, CheckFormatErr)
{
#if CHIP_CONFIG_SHORT_ERROR_STR
Expand Down

0 comments on commit 5ace630

Please sign in to comment.