forked from stratum/stratum
-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit logging of soft errors #245
Draft
ffoulkes
wants to merge
8
commits into
split-arch
Choose a base branch
from
soft-errors
base: split-arch
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5084332
Limit logging of soft errors
ffoulkes 5b166e0
Add some comments
ffoulkes 660529a
Move soft error definitions to a separate header file
ffoulkes a19273a
Expand soft error filtering
ffoulkes b3696f8
Update cmake listfile
ffoulkes 2ab1b9f
Update soft error documentation
ffoulkes 9bbc1ec
Merge branch 'split-arch' into soft-errors
ffoulkes 506cee6
Merge branch 'split-arch' into soft-errors
ffoulkes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| // Copyright 2024 Intel Corporation | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| #ifndef STRATUM_HAL_LIB_TDI_TDI_SOFT_ERROR_H_ | ||
| #define STRATUM_HAL_LIB_TDI_TDI_SOFT_ERROR_H_ | ||
|
|
||
| #include "stratum/glue/status/status.h" | ||
| #include "stratum/hal/lib/tdi/tdi_status.h" | ||
| #include "stratum/public/lib/error.h" | ||
|
|
||
| namespace stratum { | ||
| namespace hal { | ||
| namespace tdi { | ||
|
|
||
| //---------------------------------------------------------------------- | ||
| // A Soft Error is an easily recoverable condition that the application | ||
| // may not consider to be an error at all. Common examples include: | ||
| // | ||
| // - End of File (read) | ||
| // - End of Sequence (get-next) | ||
| // - Specified Item Already Exists (add) | ||
| // - Specified Item Not Found (get, remove) | ||
| // | ||
| // We treat soft errors differently by limiting the extent to which | ||
| // they are logged as the status code is passed up the stack. | ||
| // | ||
| //---------------------------------------------------------------------- | ||
|
|
||
| // Checks whether a TDI status code is a soft error. | ||
| // | ||
| // TDI_OBJECT_NOT_FOUND and TDI_TABLE_NOT_FOUND are both down-mapped | ||
| // to ERR_ENTRY_NOT_FOUND, so we treat them as indistinguishable. | ||
| static inline bool IsSoftTdiError(tdi_status_t err) { | ||
| return (err == TDI_ALREADY_EXISTS) || (err == TDI_OBJECT_NOT_FOUND) || | ||
| (err == TDI_TABLE_NOT_FOUND); | ||
| } | ||
|
|
||
| // Checks whether a Stratum error code is a soft error. | ||
| static inline bool IsSoftError(ErrorCode err) { | ||
| return (err == ERR_ENTRY_EXISTS) || (err == ERR_ENTRY_NOT_FOUND); | ||
| } | ||
|
|
||
| // Checks whether a canonical error is a soft error. | ||
| static inline bool IsSoftError(::util::error::Code err) { | ||
| return (err == ::util::error::ALREADY_EXISTS) || | ||
| (err == ::util::error::NOT_FOUND); | ||
| } | ||
|
|
||
| // Checks whether the integer error_code() value of a Status object | ||
| // is a soft error. | ||
| static inline bool IsSoftError(int err) { | ||
| return IsSoftError(static_cast<::util::error::Code>(err)); | ||
| } | ||
|
|
||
| // Special version of RETURN_IF_ERROR() that suppresses logging if this | ||
| // is a soft error. | ||
| #define RETURN_IF_ERROR_WITH_FILTERING(expr) \ | ||
| do { \ | ||
| const ::util::Status _status = (expr); \ | ||
| if (ABSL_PREDICT_FALSE(!_status.ok())) { \ | ||
| if (IsSoftError(_status.error_code())) { \ | ||
| return _status; \ | ||
| } \ | ||
| LOG(ERROR) << "Return Error: " << #expr << " failed with " << _status; \ | ||
| return _status; \ | ||
| } \ | ||
| } while (0) | ||
|
|
||
| } // namespace tdi | ||
| } // namespace hal | ||
| } // namespace stratum | ||
|
|
||
| #endif // STRATUM_HAL_LIB_TDI_TDI_SOFT_ERROR_H_ | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the definition of a soft error in this implementation? Why is "object/table not found" considered a soft error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I reviewed the updated HSD-ES ticket, I discovered that it contained log messages like this:
The commentary showed that they were also bothered about these issues (i.e., my previous commit had addressed only half their problem).
ALREADY_EXISTS and NOT_FOUND are two sides of the same coin. The conditions are easily recoverable, which is what makes them "soft" errors.
The customer is complaining about the fact that the intermediate code is logging, not just ALREADY_EXISTING, but ALREADY_EXISTING and NOT_FOUND. They are therefore complaining about our logging soft errors.
When TDI status codes are converted to Stratum status codes, TDI_TABLE_NOT_FOUND and TDI_OBJECT_NOT_FOUND are both mapped to ERR_ENTRY_NOT_FOUND. I therefore treat them as identical.
The idea is to reduce the amount of special-purpose code by treating all errors with this classification identically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point taken. I've added some additional documentation to the header file.