improve error wrapping #336
Conversation
Make sure we never return a plain syscall.Errno so that comparisons using plain == never succeed. We do this to ensure that we can influence error comparisons via errors.Is later on.
Get rid of wrapObjErr. This works by aliasing ErrNotExist to os.ErrNotExist, since errors.Is(unix.ENOENT, os.ErrNotExist) is true.
We replace some errnos from map syscalls with more descriptive
sentinel errors. Include the original errno with the error so
that both
errors.Is(err, ErrKeyNotExist)
errors.Is(err, unix.ENOENT)
work. We'll use this for other errors in the future where we
want to introduce our own errors without breaking callers that
rely on the (coarser) errno semantics.
|
@cyphar please take a look, I think this should solve your problem. |
|
Is it possible to get a new release with this patch? (In the meantime, I'll update the runc PR to use HEAD.) Also, with regards to API stability -- with the last PR I sent you mentioned you don't want to make strong guarantees about errors. Is there any stability guarantee with |
| func SyscallError(err error, errno syscall.Errno) error { | ||
| return &syscallError{err, errno} | ||
| } |
There was a problem hiding this comment.
Why not export the type and use it directly? This is what golang stdlib does with types such as os.PathError.
There was a problem hiding this comment.
I mean, instead of e.g return syscall.SyscallError(err, errno) you can do return &syscall.SyscallError{err, errno} and avoid an unnecessary function call (not sure if golang will inline it). Since this is an internal type, you're not exporting it.
The library is inconsistent in how it wraps errors from syscalls. Sometimes it obscures the underlying error, sometimes it wraps it, sometimes it replaces it with a more specific error. This PR introduces a new approach:
This approach gives users access to syscall.Errno without painting us into a corner.