Skip to content

marshal.go: updatePktSecurityParameters called with non v3 packet#314

Merged
TimRots merged 3 commits intogosnmp:masterfrom
TimRots:fix/gosnmp/updatepktsecurityparams
Mar 28, 2021
Merged

marshal.go: updatePktSecurityParameters called with non v3 packet#314
TimRots merged 3 commits intogosnmp:masterfrom
TimRots:fix/gosnmp/updatepktsecurityparams

Conversation

@TimRots
Copy link
Copy Markdown
Member

@TimRots TimRots commented Mar 24, 2021

marshal.go/sendOneRequest():

  • Add v3 PDU Report documentation
  • Add error handling for non-recoverable cases

marshal.go/send():

  • Add validation to prevent calling updatePktSecurityParameters with non v3 packet

Fixes #251 #307

Signed-off-by: Tim Rots tim.rots@protonmail.ch
Co-Authored-by: Thomas Casteleyn thomas.casteleyn@super-visions.com

@TimRots TimRots force-pushed the fix/gosnmp/updatepktsecurityparams branch 3 times, most recently from d109e5b to 5542a4c Compare March 24, 2021 22:44
Comment thread marshal.go Outdated
Comment thread marshal.go
Comment thread marshal.go Outdated
@TimRots TimRots force-pushed the fix/gosnmp/updatepktsecurityparams branch from 5542a4c to 9944a69 Compare March 25, 2021 22:36
@TimRots
Copy link
Copy Markdown
Member Author

TimRots commented Mar 25, 2021

Thanks for the first round of review @Hipska,
can you check if the updated PR covers all your remarks?

@TimRots TimRots force-pushed the fix/gosnmp/updatepktsecurityparams branch from 9944a69 to c714726 Compare March 25, 2021 23:05
Copy link
Copy Markdown
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Would it be possible to return the result anyway? Other tools then can inspect it if wanted/needed.

@TimRots
Copy link
Copy Markdown
Member Author

TimRots commented Mar 26, 2021

thanks for some more review @Hipska

Would it be possible to return the result anyway?
Other tools then can inspect it if wanted/needed.

that can be useful indeed, will update the PR.

…ection or packet

Signed-off-by: Tim Rots <tim.rots@protonmail.ch>
@TimRots TimRots force-pushed the fix/gosnmp/updatepktsecurityparams branch from c714726 to 22f2d7f Compare March 26, 2021 11:07
Signed-off-by: Tim Rots <tim.rots@protonmail.ch>
@TimRots TimRots requested a review from SuperQ March 26, 2021 11:13
@Hipska
Copy link
Copy Markdown
Contributor

Hipska commented Mar 26, 2021

Can you link #251 to be fixed by this PR as well?

Copy link
Copy Markdown
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Some minor nits, otherwise great.

Comment thread marshal.go Outdated
Comment thread marshal.go Outdated
Comment thread marshal.go Outdated
Comment thread marshal.go Outdated
@TimRots TimRots force-pushed the fix/gosnmp/updatepktsecurityparams branch 2 times, most recently from 6047f87 to 30b89d5 Compare March 28, 2021 21:45
Signed-off-by: Tim Rots <tim.rots@protonmail.ch>
@TimRots TimRots force-pushed the fix/gosnmp/updatepktsecurityparams branch from 30b89d5 to 792a7e5 Compare March 28, 2021 22:13
Copy link
Copy Markdown
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM, nice.

@TimRots
Copy link
Copy Markdown
Member Author

TimRots commented Mar 28, 2021

Thanks for the reviews gents 👍

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.

updatePktSecurityParameters called with non Version3 connection or packet updatePktSecurityParameters called with non Version3 connection or packet

3 participants