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

Add test of v1model update_checksum implementation #882

Merged
merged 2 commits into from
Sep 6, 2017
Merged

Add test of v1model update_checksum implementation #882

merged 2 commits into from
Sep 6, 2017

Conversation

jafingerhut
Copy link
Contributor

It does not test the verify_checksum implementation.

It does not test the verify_checksum implementation.
@jafingerhut
Copy link
Contributor Author

jafingerhut commented Sep 4, 2017

The new P4 program only compiles with the new v1model checksum APIs added via this commit: f09a95d

but later reverted on 2017-Aug-28.

In my fork of the p4c repo, I created a branch from the commit just before the changes were reverted on 2017-Aug-28, but when I created a PR on Github from that branch, Github seems to be treating it as a commit at the top of master, and hence the program fails to compile because verify_checksum() and update_checksum() are not defined.

Copy link
Contributor

@cc10512 cc10512 left a comment

Choose a reason for hiding this comment

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

Thank you for putting this together. Since it is failing on master, can you please add the test to the list on known failures (backends/bmv2/CMakeLists.txt:106 and backends/p4test/CMakeLists.txt:52)? We'll remove it when we merge back the checksum support.

@mihaibudiu
Copy link
Contributor

There should already be a few tests with stf files that exercise the checksums in bmv2.
But more tests are always better.

@jafingerhut
Copy link
Contributor Author

@mbudiu-vmw I found only one P4_16 test program in the p4c/testdata/p4_16_samples program where there is checksum calculation code, and there is an .stf file to specify input packets, and expected output packets that check for the correct checksum being computed. That is issue655-bmv2.p4 and issue655-bmv2.stf. It only calculates checksums for a single 16-bit field.

The test program and .stf file proposed in this PR calculate the IPv4 header checksum over an entire 20-byte IPv4 header, plus a test packet that has 24-bit IPv4 header with options, parsed into a varbit field with a non-0 length. Thus this test should be adding a little bit of functional coverage not already covered by other tests.

@jafingerhut
Copy link
Contributor Author

@cc10512 I have tried making the changes you suggested -- thanks for file name and line number pointers to guide me. We'll see what the Travis build thinks of these changes in a bit.

@cc10512 cc10512 merged commit 91f9fef into p4lang:master Sep 6, 2017
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.

3 participants