-
Notifications
You must be signed in to change notification settings - Fork 42
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
check if we can decode an error before trying #108
Conversation
Thanks for the fast response! |
cli/parse.go
Outdated
@@ -15,7 +15,7 @@ import ( | |||
"github.com/ipfs/go-ipfs-cmds" | |||
logging "github.com/ipfs/go-log" | |||
|
|||
osh "gx/ipfs/QmXuBJ7DR6k3rmUEKtvVMhwjmXDuJgXXPUt4LQXKBMsU93/go-os-helper" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something had gone rouge here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think importing the github path is okay, and the package is in the gx deps. I think I would remove the blank line above, though, or move logging below the blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keks fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for stalling this that long. Are you sure checking res.dec == nil
is enough? I think there are cases where this is valid state.
@@ -210,6 +210,8 @@ func parseResponse(httpRes *http.Response, req *cmds.Request) (cmds.Response, er | |||
} | |||
e.Message = string(mes) | |||
e.Code = cmdkit.ErrNormal | |||
case res.dec == nil: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we also use res.dec == nil
to check whether the response is a io.Reader
-style byte stream here:
Line 59 in 66802ba
if res.dec == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a raw bytes error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This switch is trying to decode an error. Currently, it handles plain-text error responses, and "encoded" error responses (e.g., JSON). I don't think there are any valid cases where res.dec
is nil
and contentType
isn't plain (for errors, at least).
cli/parse.go
Outdated
@@ -15,7 +15,7 @@ import ( | |||
"github.com/ipfs/go-ipfs-cmds" | |||
logging "github.com/ipfs/go-log" | |||
|
|||
osh "gx/ipfs/QmXuBJ7DR6k3rmUEKtvVMhwjmXDuJgXXPUt4LQXKBMsU93/go-os-helper" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think importing the github path is okay, and the package is in the gx deps. I think I would remove the blank line above, though, or move logging below the blank line.
avoids a nil pointer dereference fixes ipfs/kubo#4998
avoids a nil pointer dereference
fixes ipfs/kubo#4998