libcontainer: rm own error system #3033
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
kolyshkin
force-pushed
the
rm-own-errors
branch
4 times, most recently
from
e6fb240
to
35d2615
Compare
kolyshkin
changed the title
|
CI failure is probably a flake (#3035); CI restarted |
It is not used since commit 244c9fc. Signed-off-by: Kir Kolyshkin <[email protected]>
|
Rebased on top of #3011 |
|
With removal of generic_error.go, probably remove |
kolyshkin
commented
Jun 24, 2021
| @@ -74,71 +74,39 @@ type BaseContainer interface { | |||
| ID() string | |||
|
|
|||
| // Returns the current status of the container. | |||
| // | |||
There was a problem hiding this comment.
This documentation (about which errors can be returned) is removed because
- libcontainer/error.go already list possible errors;
- it's not easy to keep these lists up to date.
This removes libcontainer's own error wrapping system, consisting of a few types and functions, aimed at typization, wrapping and unwrapping of errors, as well as saving error stack traces. Since Go 1.13 now provides its own error wrapping mechanism and a few related functions, it makes sense to switch to it. While doing that, improve some error messages so that they start with "error", "unable to", or "can't". A few things that are worth mentioning: 1. We lose stack traces (which were never shown anyway). 2. Users of libcontainer that relied on particular errors (like ContainerNotExists) need to switch to using errors.Is with the new errors defined in error.go. 3. encoding/json is unable to unmarshal the built-in error type, so we have to introduce initError and wrap the errors into it (basically passing the error as a string). This is the same as it was before, just a tad simpler (actually the initError is a type that got removed in commit afa8443; also suddenly ierr variable name makes sense now). Signed-off-by: Kir Kolyshkin <[email protected]>
This removes the libcontainer/stacktrace package, which, as of previous commit, is no longer used. Signed-off-by: Kir Kolyshkin <[email protected]>
Thanks, @zhsj! Removed. |
AkihiroSuda
reviewed
Jul 1, 2021
| } | ||
| if err := l.validateID(id); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := l.Validator.Validate(config); err != nil { | ||
| return nil, newGenericError(err, ConfigInvalid) | ||
| return nil, &ConfigError{err.Error()} |
There was a problem hiding this comment.
Yeah, this looks a tad weird. I might change this to using an error later.
AkihiroSuda
reviewed
Jul 1, 2021
| @@ -249,28 +251,28 @@ type LinuxFactory struct { | |||
|
|
|||
| func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, error) { | |||
| if l.Root == "" { | |||
| return nil, newGenericError(errors.New("invalid root"), ConfigInvalid) | |||
| return nil, &ConfigError{"invalid root"} | |||
There was a problem hiding this comment.
It's a replacement for "ConfigInvalid" error code, so a caller can distinguish configuration errors from everything else.
There was a problem hiding this comment.
You're right @AkihiroSuda, it's better not to have it, but I was not able to come with a good solution before. Now I am; please see #3056
AkihiroSuda
approved these changes
Jul 1, 2021
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 removes libcontainer's own error wrapping system, consisting of a
few types and functions, aimed at typization, wrapping and unwrapping
of errors, as well as saving error stack traces.
Since Go 1.13 now provides its own error wrapping mechanism and a few
related functions, it makes sense to switch to it.
A few things worth mentioning:
We lose stack traces (which were never shown anyway).
Users of libcontainer that relied on particular errors (like
ContainerNotExists) need to switch to usingerrors.Iswiththe new errors defined in libcontainer/error.go.
An example of such conversion:
encoding/jsonis unable to unmarshal the built-inerror type, so we have to introduce
initErrorand wrap the errors into it(basically passing the error as a string). This is the same
as it was before, just a tad simpler (actually the
initErroritself is a type that got removed in commit afa8443; also suddenly
ierrvariable name makes sense now).Proposed changelog entry
libcontainer.Errortype, together with error codes, is removed in favor of new errors. Users relying on removed types and codes should adopt to useerrors.Iswith new errors (seelibcontainer/errors.go). (libcontainer: rm own error system #3033)