refactor sshfx encoding, fix link rot, go fmt#545
Conversation
| *b = Buffer{ | ||
| b: b.b[:0], | ||
| } |
There was a problem hiding this comment.
This syntax clears off and Err implicitly.
|
|
||
| if b.Len() < 1 { | ||
| return 0, ErrShortPacket | ||
| b.off = len(b.b) |
There was a problem hiding this comment.
We set the b.off to len(b.b) before every b.Err = ErrShortPacket so that say, we’re unmarshalling a uint32, and that fails because there are only 3 bytes left, but then we unmarshal a uint8, which would succeed and consume one of the three remaining bytes.
We also if b.Err != nil { … } as preamble of each Consume… function, but it’s better to have safety in layers, rather than rely on only one single failsafe.
|
|
||
| v := b.b[b.off:] | ||
| if len(v) > int(length) { | ||
| if len(v) > length || cap(v) > length { |
There was a problem hiding this comment.
I realized, we should ensure the cap is clamped the same as len.
| // | ||
| // If hint has sufficient capacity to hold the data, it will be reused and overwritten, | ||
| // otherwise a new backing slice will be allocated and returned. | ||
| func (b *Buffer) ConsumeByteSliceCopy(hint []byte) []byte { |
There was a problem hiding this comment.
Turns out, we were using this code twice, but it was also wholly reallocating if len was insufficient, even though maybe it has the available cap. So, using the grow slice trick instead, so maybe we have a chance to reuse hint even if the len is insufficient, but the cap is sufficient.
| @@ -1,4 +1,4 @@ | |||
| package filexfer | |||
| package sshfx | |||
There was a problem hiding this comment.
Changing the expected name of this package, so that importers know the preferred name it should be renamed to.
| } | ||
| } | ||
|
|
||
| func newPacketFromType(typ PacketType) (Packet, error) { |
There was a problem hiding this comment.
This function makes way more sense here, rather than in packets.go.
| Handle string | ||
| Offset uint64 | ||
| Len uint32 | ||
| Length uint32 |
There was a problem hiding this comment.
Name change to better mirror standard.
| ) | ||
|
|
||
| const extensionPosixRename = "posix-rename@openssh.com" | ||
| const extensionPOSIXRename = "posix-rename@openssh.com" |
There was a problem hiding this comment.
POSIX is an all-caps abbreviation that should be all-caps in variable names. (Are you glad we had this all in internal so we could change this bad early draft choice? 
)
drakkan
left a comment
drakkan
left a comment
There was a problem hiding this comment.
I took a quick look and it looks ok. I don't have time for a full review, sorry. In general I may reduce my involvement in open source in the coming months.
|
It’s ok. I’ve had to scale back from time to time as well. Especially, when a job comes in that doesn’t have a synergy with working on the project. |
2 participants
I’ve poked around with the wire formatting code, and switched from a “check error every time” to “accumulate an error” which allows for more direct and concise unmarshals in general.
Also, I found the ietf draft link rot #544 and updated each of the ones that points to a version earlier than
-13. (The-13links are still valid.)Also, we’ve accumulated some
go fmtchanges that show up in go 1.20, so I threw those in as well.