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

[ovsp4rt] DPDK PrepareFdbTxVlanTableEntry truncates vlan_id #689

Open
ffoulkes opened this issue Sep 16, 2024 · 0 comments
Open

[ovsp4rt] DPDK PrepareFdbTxVlanTableEntry truncates vlan_id #689

ffoulkes opened this issue Sep 16, 2024 · 0 comments

Comments

@ffoulkes
Copy link
Contributor

ffoulkes commented Sep 16, 2024

Value semantics

The function subtracts 1 from vlan_id and uses it as port_id.

  • There is no comment explaining why ovsp4rt uses (vlan_id - 1) as the port_id.
  • There is also no explanation of why the transformation is done in ovsp4rt instead of OvS.
  • The function should guard against a vlan_id of 0, which would cause the value to wrap.
  • An unconventional operation such as this warrants its own Encode function.

There are, at heart, two questions:

  • WHY is the function using vlan_id - 1 as a port_id?
  • WHAT parts of the software are (or should be) aware of this, and what parts of the software are not or should not?

If the knowledge belongs solely to OvS, then ovs should set an appropriately-named field in the parameter struct, and ovsp4rt should handle it opaquely.

If it is appropriate for ovsp4rt to make the transformation, then it should be encapsulated in an Encode function (separating
the value semantics from the Prepare function, whose chief purpose is to populate the Request protobuf), and the Encode function should have a comment explaining what's going on, and why.

Field width

The transformed vlan_id (nominally bit<12>) is encoded as a single byte (bit<8>), losing the high-order four bits.

The value should be encoded as two bytes, or there should be a comment explaining why it is correct to truncate the value.

See also: #683.

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

No branches or pull requests

1 participant