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

computestorage APIs stringify their error instead of wrapping it #924

Closed
TBBle opened this issue Jan 14, 2021 · 1 comment · Fixed by #925
Closed

computestorage APIs stringify their error instead of wrapping it #924

TBBle opened this issue Jan 14, 2021 · 1 comment · Fixed by #925

Comments

@TBBle
Copy link
Contributor

TBBle commented Jan 14, 2021

Reference: microsoft/go-winio#187 (comment)

In a unit test, I had code that checked an API call failure to detect missing privilege:

if errno, ok := errors.Cause(err).(syscall.Errno); ok && errno == syscall.ERROR_PRIVILEGE_NOT_HELD {

however, adapting it to use computestorage.FormatWritableLayerVhd, that no longer works, and I must use

if strings.Contains(err.Error(), "A required privilege is not held by the client.") {

This is because of the use of fmt.Errorf('...%s', err). If this is changed to errors.Wrapf(err, '...'), then I'd get the result I wanted.

I guess this was an oversight, since computerstorage/helpers.go does use errors.Wrapf and errors.Wrap.

However, I don't want to just put up a PR without discussion, if this was deliberately done to hide/obscure the underlying failure in these APIs.

@dcantah
Copy link
Contributor

dcantah commented Jan 14, 2021

@TBBle Was an oversight, I can send a change for this. Thanks!

dcantah added a commit to dcantah/hcsshim that referenced this issue Jan 14, 2021
* Change from stringifying errors with fmt.Errorf to errors.Wrap everywhere

Fixes: microsoft#924

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
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 a pull request may close this issue.

2 participants