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.
Value semantics
The function subtracts 1 from vlan_id and uses it as port_id.
There are, at heart, two questions:
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.