byteorder: Simplify byteorder package #16201
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twpayne
added
area/misc
|
test-me-please |
|
CI is failing due to the current GitHub Actions outage. |
brb
reviewed
May 18, 2021
aanm
approved these changes
May 19, 2021
pkg/byteorder/byteorder.go
Outdated
| return fmt.Sprintf("unsupported type(%v): %v", reflect.TypeOf(field).Kind(), field) | ||
| // HostSliceToNetwork32 converts b to the networking byte order. | ||
| func HostSliceToNetwork32(b []byte) uint32 { | ||
| return Native.Uint32(b[len(b)-4:]) |
There was a problem hiding this comment.
why not _ = b[3] to assert the len of b?
There was a problem hiding this comment.
Good idea. And it turns out that this (and the original HostSliceToNetwork function) is completely wrong. Updated PR coming up...
|
test-me-please |
aanm
approved these changes
May 19, 2021
tklauser
approved these changes
May 20, 2021
christarazi
added
the
kind/enhancement
jibi
approved these changes
May 21, 2021
twpayne
removed
the
dont-merge/wait-until-release
|
test-me-please |
|
ci-aks |
|
ci-gke |
|
test-me-please |
2 similar comments
|
test-me-please |
|
test-me-please |
errordeveloper
approved these changes
May 26, 2021
The previous HostToNetwork and NetworkToHost functions used a combination of the unsafe package, reflection, unchecked type assertions, and panics. Replace them with simple functions controlled by build tags. Signed-off-by: Tom Payne <tom@isovalent.com>
Signed-off-by: Tom Payne <tom@isovalent.com>
The previous HostSliceToNetwork function used a combination of reflection, unchecked type assertions, and panics to claim to convert a slice of bytes in host order to a uint32 in network byte order on machines of any endianness. What actually happened was quite different. HostToSliceNetwork was called with net.IP types which are []bytes of length 4 or 16 and are already in network order with the IPv4 part of the address in the last four bytes. On little-endian machines the following happened: The call to reverse in the little endian path had the side effect of not only reversing the order of bytes in the slice, but also moving the IPv4 address in the last four bytes of the slice to first four bytes of the slice. The call to binary.BigEndian.Uint32 (note the hardcoded BigEndian) does not swap the bytes. So, the net effect was that it was actually used to convert the last four bytes of a slice in network byte order to a uint32 in host order. This was even visible in the tests where the IP address 10.11.129.91 was converted to 0x5b810b0a - note the position of the 10. On big-endian machines the code would have sporadically failed. Length 4 net.IPs would have been correctly left unchanged, but length 16 net.IPs would have their first four bytes used, all of which would be zero, causing HostToSliceNetwork to always return zero. In any case, the tests were hard-coded with values that would only work on little-endian machines. This commit replaces HostToNetworkSlice with NetIPv4ToHost32 to more accurately reflect both what the function does and how it is used. Signed-off-by: Tom Payne <tom@isovalent.com>
|
test-me-please |
|
ci-aks Failure is #16292. |
|
test-gke New flake I think: #16337 |
|
test-1.21-4.9 Could be #14959, but not sure. |
|
test-gke |
|
test-1.21-4.9 |
|
Hmm, the two previous comments don't seem to have re-triggered the relevant tests. Specifically, clicking on "Details" for the failing tests links to yesterday's runs. Will try again. |
|
test-gke |
|
test-1.21-4.9 |
1 similar comment
|
test-1.21-4.9 |
|
test-gke |
1 similar comment
|
test-gke |
|
ci-aks |
|
test-1.21-4.9 failure is flake #16237. Almost all checks have passed, and the change is small, so marking this as |
twpayne
added
the
ready-to-merge
kaworu
approved these changes
May 31, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pkg/byteorderintroduced in #730 uses a mix ofunsafe, reflection, unchecked type assertions, and panics to handle byte order conversions, and the tests would only pass on little endian machines.This PR replaces the package with much simpler functions and fixes the tests.
As this PR touches several parts of the code it should probably only be merged once 1.10 is released.