Skip to content

fix(survey): Check for nil PeerID before deref - #1120

Open
placer14 wants to merge 1 commit into
masterfrom
mg/fix/1119-minerinfo-deref-nil
Open

fix(survey): Check for nil PeerID before deref#1120
placer14 wants to merge 1 commit into
masterfrom
mg/fix/1119-minerinfo-deref-nil

Conversation

@placer14

Copy link
Copy Markdown
Contributor

Check for nil minerinfo.PeerId before deref.

fixes #1119

@placer14
placer14 requested a review from frrist January 29, 2023 23:45
@placer14

placer14 commented Jan 29, 2023

Copy link
Copy Markdown
Contributor Author

I'm unsure why the API would return a nil PeerId when the error is nil. Skipping the persistence of these incomplete models seems reasonable. LMK if I should go a different direction with this.

Comment thread tasks/survey/minerprotocols/minerprotocols.go Outdated
@placer14
placer14 force-pushed the mg/fix/1119-minerinfo-deref-nil branch from 4f951f2 to be146e2 Compare January 30, 2023 03:41
@placer14
placer14 force-pushed the mg/fix/1119-minerinfo-deref-nil branch from be146e2 to 06a37b0 Compare January 30, 2023 03:43
Comment on lines 149 to -151
var peerID string
if minerInfo.PeerId != nil {
peerID = minerInfo.PeerId.String()

@frrist frrist Jan 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this would be more clear if this check were moved into getMinerAddrInfo. If the peerID is nil that method can just return and log an error. What do you think @placer14?

@frrist frrist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, please address feedback and then merge

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.

Task minerprotocols causes panic

3 participants