[ovsp4rt] Update C++ API functions to return absl::Status #734
Conversation
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
- Renamed the functions in the internal C++ API from ConfigEntry to DoConfigEntry, to keep them from being confused with ConfigTableEntry functions. The conventions are: - A PrepareTableEntry function populates the fields of a TableEntry. - A ConfigTableEntry function initializes a WriteRequest, prepares the TableEntry, and sends the request to the P4Runtime server. - A GetTableEntry function initializes a ReadRequest, prepares the TableEntry, sends the request to the P4Runtime server, and returns the response. - A DoConfigEntry function implements the core logic of an ovsp4rt_table_entry C API. It may make one or more ConfigTableEntry or GetTableEntry calls. Signed-off-by: Derek Foster <[email protected]>
Signed-off-by: Derek Foster <[email protected]>
Signed-off-by: Derek Foster <[email protected]>
ffoulkes
requested review from
5abeel,
n-sandeep and
vsureshkumarp
as code owners
- Modified the functions in the internal C++ API to return absl::Status. Explicitly ignored return status in the C API wrappers. Status is currently for use in testing. Note that issue #618 proposes that public APIs should return status. - Moved ovsp4rt_str_to_tunnel_type() to a separate source file for C api functions that are not wrappers. Signed-off-by: Derek Foster <[email protected]>
ffoulkes
force-pushed
the
return-api-status
branch
from
ee1d642 to
1317ec2
Compare
ffoulkes
changed the title
ffoulkes
changed the title
ffoulkes
changed the title
Signed-off-by: Derek Foster <[email protected]>
5abeel
reviewed
Jan 9, 2025
| // Ignore errors (why?) | ||
| } | ||
| // Ignore errors (why?) | ||
| (void)ConfigFdbTunnelTableEntry(client, learn_info, p4info, insert_entry); |
There was a problem hiding this comment.
Is the (void) addition to suppress compiler warnings?
There was a problem hiding this comment.
Yes. It is also a way of telling the reader that we are ignoring the value returned by the function.
The comment is shorthand for "the original developer did this, and it's probably a defect; but we don't know that for sure, and we either don't have the time or are too scared to change it right now. We will, however, leave this comment as a sort of a 'danger, thin ice' warning, for the benefit of future developers." It's kind of like inserting [sic] into a document you're editing.🙄
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
absl::Status, allowing test programs to verify whether the function encountered an error.voidreturn type.These changes only apply when BUILD_CLIENT is enabled.
Note
Issue #618 proposes that the public APIs should return status. The Status returned by the core APIs will give them the means to do so, should we decide to implement this change.