On Sun, 13 Aug 2017 21:13:57 +0000, Jan Scheurich wrote: > Jiri, I am not too familiar with conventions on the OVS netlink > interface regarding the handling of variable length fields. What is > the benefit of structuring the push_nsh action into > > OVS_ACTION_ATTR_PUSH_NSH > +-- OVS_ACTION_ATTR_NSH_BASE_HEADER > +-- OVS_ACTION_ATTR_NSH_MD > > instead of grouping the base header fields and the variable length MD > into a single monolithic attribute OVS_ACTION_ATTR_PUSH_NSH? Is the > main concern a potential future need to add additional parameters to > the push_nsh datapath action? Are there examples for such structured > actions other than OVS_ACTION_ATTR_CT where the need is obvious > because it is very polymorphic?
This is about having the design clean. What would you do if you had two variable length fields? Would you still put them in a single structure and have a length field in the structure, too? That would be wrong, we have length in the attribute header. I doubt you would do that. Which indicates that putting variable length fields to an attribute with anything other is wrong. Also, look at how ugly the code would be. You'd have to subtract the base header length from the attribute length to get the variable data length. That's not nice at all. Think about the netlink in the way that by default, every field should have its own attribute. Structures are included only for performance reasons where certain fields are always passed in a message and having them in separate attributes would be impractical and waste of space. Going out of your way to include the context data in the structure thus doesn't make sense. > BTW: The name OVS_ACTION_ATTR_NSH_BASE_HEADER is misleading because > in the NSH draft the term base header is used for the first 32-bit > word, whereas here it includes also the 32-bit Service Path header. An excellent comment. This means that it's very well possible that future NSH versions may not include SP header or may have it of a different size. Maybe we should consider putting it into a separate attribute, too? Not sure it is needed, though, I don't think it's likely to happen. Jiri