Expose string errors from dqlite_node_create #381
Merged
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
In dqlite_node_create, we no longer free the `dqlite_node *` argument and set it to NULL in the event of an error. Instead, we document that callers are responsible for calling dqlite_node_destroy even when creation fails. `struct dqlite_node` gains an additional bit of state to allow dqlite_node_destroy to gracefully handle uninitialized arguments. With these changes made, dqlite_node_create can use the errmsg channel to report human-readable errors, in line with other functions in dqlite.h (see next commit). Signed-off-by: Cole Miller <cole.miller@canonical.com>
|
I'll let @freeekanayaka and @MathieuBordere weight in on this one. |
freeekanayaka
approved these changes
Aug 25, 2022
It seems like unnecessary coupling to use the raft constant everywhere, plus having different names makes it harder to confuse the two errmsg buffers involved. Signed-off-by: Cole Miller <cole.miller@canonical.com>
Signed-off-by: Cole Miller <cole.miller@canonical.com>
|
@cole-miller do you expect go-dqlite to behave weirdly if I merge this? |
|
We'll want to update go-dqlite to revert my canonical/go-dqlite#196, but it should be okay to merge this first -- it won't change go-dqlite's externally visible behavior. |
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.
This implements roughly @freeekanayaka's suggestion in canonical/raft#228. If we agree this is the way to go, I'll make a PR to go-dqlite to adjust its behavior accordingly.
Note that unlike Free's sample code in that thread, I don't differentiate in the public API between DQLITE_NOMEM errors and other kinds of errors. Instead, dqlite_node_errmsg and dqlite_node_destroy just handle NULL arguments gracefully. I went this route because it makes callers' lives simpler and because DQLITE_NOMEM is an ambiguous signal -- was it the
struct dqlite_nodeallocation that failed, or some other allocation along the way?