Skip to content
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

Use errors.Unwrap() where possible #2280

Merged
merged 6 commits into from
Apr 2, 2020

Conversation

kolyshkin
Copy link
Contributor

This set makes use of errors.Unwrap() where possible to get to an underlying error. The biggest motivation is to simplify the code.

The feature requires go 1.13 but since merging #2256 we are already not supporting go 1.12 (which is an unsupported release anyway).

This simplifies code a lot.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Tested that the EINTR is still being detected:

> $ go1.14 test -c # 1.14 is needed for EINTR to happen
> $ sudo ./fscommon.test
> INFO[0000] interrupted while writing 1063068 to /sys/fs/cgroup/memory/test-eint-89293785/memory.limit_in_bytes
> PASS

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Apr 1, 2020

Let me know if you want me to merge most of the commits (or all of them)

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 1, 2020

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael
Copy link
Member

crosbymichael commented Apr 2, 2020

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit f8e1388 into opencontainers:master Apr 2, 2020
@kolyshkin
Copy link
Contributor Author

Oh well, this might not be working as I thought it would. See this example: https://play.golang.org/p/-HyjwI4v_pz

IOW if an error is not wrapped, errors.Unwrap() returns nil! This is nuts :-\

@AkihiroSuda
Copy link
Member

We should revert this PR?

errno = err.Err
}
// Handle some specific syscall errors.
errno := errors.Unwrap(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code might not be working the way it should. If err is not wrapped, errno will be nil.

Will do a followup PR.

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Apr 3, 2020
…unwrap"

Using errors.Unwrap() is not the best thing to do, since it returns
nil in case of an error which was not wrapped. More to say,
errors package provides more elegant ways to check for underlying
errors, such as errors.As() and errors.Is().

This reverts commit f8e1388, reversing
changes made to 6ca9d8e.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

We should revert this PR?

@AkihiroSuda while I don't see any code paths that might hit the problem I described above, it's better to not use errors.Unwrap() directly, there are better ways. Please see #2291.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants