Bug 1990130

Summary: Be more assertive with tc_policy parameter
Product: Red Hat Enterprise Linux Fast Datapath Reporter: Marcelo Ricardo Leitner <mleitner>
Component: openvswitchAssignee: Michael Santana <msantana>
openvswitch sub component: ovs-hw-offload QA Contact: qding
Status: NEW --- Docs Contact:
Severity: medium    
Priority: medium CC: ctrautma, fleitner, mhou, msantana, qding
Version: FDP 21.F   
Target Milestone: ---   
Target Release: FDP 21.G   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 2172625    

Description Marcelo Ricardo Leitner 2021-08-04 19:55:02 UTC 
Description of problem:
It currently accepts any value, and when it fails to parse it, it assumes "none" (because NONE is 0).

It is probably aggravated by the switching between "-" in "hw-offload" to "_" in "skip_X".

There is a warning in there, but it can easily go unnoticed like in
https://bugzilla.redhat.com/show_bug.cgi?id=1975085#c22
(This is not the first time I've seen this happening)

AFAIK we need ovs running to change that value, so we can't have ovs fail to start. But it would be good if we could improve this somehow. Perhaps block the "set" command to only known values?

  void
  tc_set_policy(const char *policy)
  {
      if (!policy) {
          return;
      }

      if (!strcmp(policy, "skip_sw")) {
          tc_policy = TC_POLICY_SKIP_SW;
      } else if (!strcmp(policy, "skip_hw")) {
          tc_policy = TC_POLICY_SKIP_HW;
      } else if (!strcmp(policy, "none")) {
          tc_policy = TC_POLICY_NONE;
      } else {
          VLOG_WARN("tc: Invalid policy '%s'", policy);
          return;
      }

      VLOG_INFO("tc: Using policy '%s'", policy);
  }
Comment 4 Marcelo Ricardo Leitner 2022-09-30 18:50:11 UTC 
Any news here?
Comment 6 Michael Santana 2023-01-07 19:27:14 UTC 
Hi,

Yes, I would love to make this a bit more strict with the arguments it takes in. The issue does not lie alone with the tc policy argument. It happens in all other_config configurations


A typical ovs set command looks like this
ovs-vsctl set Open_vSwitch . other_config:KEY=VALUE

The problem is that KEY and VALUE can be any arbitrary string and ovs-vsctl will happily accept it and not throw any warnings. Ideally ovs-vsctl should look up KEY and make sure it is valid and then make sure VALUE is valid for that particular KEY, otherwise throw an error.

We know that the "set" command does do some blocking, i.g. if you misspell "other_config" in the above command you will get an error about it being an invalid column. other_config is so well documented at this point with all the key:value pairs that there shouldnt be any reason why we couldnt be any strict about it

Let me look how to implement this in the code. We might need to make a lookup table with all the key:value pairs
Comment 7 Michael Santana 2023-05-24 16:33:06 UTC 
*** Bug 2208960 has been marked as a duplicate of this bug. ***