Skip to content
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

Fixed payload size issue #659

Merged
merged 4 commits into from
Apr 13, 2022
Merged

Conversation

Akshata440
Copy link
Contributor

@Akshata440 Akshata440 commented Apr 12, 2022

What does this Pull Request accomplish?

Changes made to fix the issue #658

releases\1.5 branch will be fast-forwarded to include this fix and minor/patch release will be published with this bug fix as it fixes showstopper bug for ENET frames in XNET.

Why should this Pull Request be merged?

While calling WriteFrame API, we need to pass in buffer with last 4 bytes of buffer reserved for adding FCS (frame checksum). in gRPC service we take payload data as input from user and allocate buffer and copy payload that user has sent in. Earlier we had missed allocating extra 4 bytes for the FCS while allocating buffer for copying data. Hence, 4 bytes of the payload were not being written correctly. I have added a new constant called ETHERNET_FRAME_FCS_SIZE which is now used for calculating payload sizes while reading and writing frames.

What testing has been done?

Ran system tests in #654 to confirm the whole payload is read correctly.
Before :
image

After :
image

@doshirohan doshirohan linked an issue Apr 12, 2022 that may be closed by this pull request
@Akshata440 Akshata440 marked this pull request as ready for review April 12, 2022 11:31
@mgumble
Copy link
Contributor

mgumble commented Apr 12, 2022

Well this is a major bummer, releasing XNET with this bug. @asumit do you think we just didn't do enough manual testing to catch this before creating the release?

@Akshata440 You mentioned we would fast forward the release branch, but we need to update the artifacts too and create yet another release I guess with a minor version bump?
@astarche Thoughts on versioning?

@astarche
Copy link
Collaborator

The version should be 1.5.1. The idea of fast-forwarding and using the same branch sounds right to me as well.

@doshirohan
Copy link
Contributor

Well this is a major bummer, releasing XNET with this bug. @asumit do you think we just didn't do enough manual testing to catch this before creating the release?
@Akshata440 You mentioned we would fast forward the release branch, but we need to update the artifacts too and create yet another release I guess with a minor version bump?

We did test the API manually and we were in process of automating that via test when we encountered the issue. During testing, we had used XNET C API for generating frames and were reading those gRPC API in loopback test to validate the data. This had passed, though this issue had turned out to be in write frame itself which we couldn't catch with that manual test.

We are planning to create 1.5.1 patch release for this once the PR goes in.

@Akshata440 @mgumble @astarche @reckenro @asumit

@Akshata440 Akshata440 merged commit 8a779ac into ni:main Apr 13, 2022
@Akshata440 Akshata440 deleted the users/akk/fix-payload-size branch April 13, 2022 06:13
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.

Ethernet frame is missing last four bytes in the payload
5 participants