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

Do not expect or emit padded byte strings in BFRT P4Runtime translator #938

Merged
merged 6 commits into from
Mar 22, 2022

Conversation

Yi-Tseng
Copy link
Contributor

@Yi-Tseng Yi-Tseng commented Mar 21, 2022

  • No need to add padding zeros when writing through the SDE wrapper since it will help add zeros before writing to the SDE
  • Values from SDE wrapper for port number can be 1 or 2 bytes when we set incompatible_enable_bfrt_legacy_bytestring_responses to false

 - No need to add padding zeros when writing to the low level driver
 - Values from low level driver for port number can be 1 or 2 bytes
@pudelkoM
Copy link
Member

low level driver being the SDE wrapper here? Or does the SDE accept truncated strings now?

@Yi-Tseng
Copy link
Contributor Author

low level driver being the SDE wrapper here? Or does the SDE accept truncated strings now?

I mean SDE accepts truncated byte strings
I tested with SDE 9.7.0 and didn't see issues.

@pudelkoM
Copy link
Member

Did you test the SDE with custom code? Because all translated entries still go through the wrapper, which does the padding. Either way, the wrapper will take care of it, no need to pad here.

@Yi-Tseng
Copy link
Contributor Author

Did you test the SDE with custom code? Because all translated entries still go through the wrapper, which does the padding. Either way, the wrapper will take care of it, no need to pad here.

I was wrong, I didn't realized that the wrapper adds padding zeros before sending to the SDE

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #938 (4b7d0a3) into main (9b2dc9a) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #938      +/-   ##
==========================================
- Coverage   79.20%   79.18%   -0.02%     
==========================================
  Files         339      339              
  Lines       30928    30924       -4     
==========================================
- Hits        24495    24488       -7     
- Misses       6433     6436       +3     
Impacted Files Coverage Δ
...atum/hal/lib/barefoot/bfrt_p4runtime_translator.cc 96.84% <66.66%> (-0.66%) ⬇️

@pudelkoM
Copy link
Member

Did you test the SDE with custom code? Because all translated entries still go through the wrapper, which does the padding. Either way, the wrapper will take care of it, no need to pad here.

I was wrong, I didn't realized that the wrapper adds padding zeros before sending to the SDE

No worries, I wasn't sure if the SDE changed either.

@pudelkoM pudelkoM changed the title Fix P4Runtime translation byte string size Do not expect or emit padded byte strings in BFRT P4Runtime translator Mar 22, 2022
@pudelkoM pudelkoM merged commit b54ade9 into main Mar 22, 2022
@pudelkoM pudelkoM deleted the fix-p4rt-xlation-check branch March 22, 2022 22:10
ffoulkes added a commit to ipdk-io/stratum-dev that referenced this pull request Jan 17, 2024
…um#938)

Signed-off-by: Derek G Foster <derek.foster@intel.com>
ffoulkes added a commit to ipdk-io/stratum-dev that referenced this pull request Jan 17, 2024
…um#938)

Signed-off-by: Derek G Foster <derek.foster@intel.com>
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.

None yet

2 participants