On Mon, 21 Aug 2017 14:11:10 +0800, Yang, Yi wrote: > In OVS code, it has been removed because of Microsoft compiler issue.
We absolutely, completely and utterly do not care in the kernel. Please never make such arguments and never make the code look worse because of a compiler for other operating systems. > > I was wondering about this during the reviews of the previous versions. > > Now I've given this more thought but I still don't see it - why is the > > inner_protocol set here? > > I saw push_mpls has it, so also set it. MPLS supports GSO and needs this for segmentation. I don't see anything GSO related in this patch. How do you plan to address GSO, anyway? > > > + err = check_header(skb, length); > > > + if (unlikely(err)) > > > + return err; > > > + > > > + key->nsh.flags = nsh_get_flags(nsh); > > > > Again, need to reload nsh. > > I used skb->len in v5, so we can't avoid such issue. And how do you ensure that the skb has enough headroom, then? That is wrong. All I said is that you have to reload the pointers to the header which is what you have to do. > > Wouldn't it be nicer if the fields of struct ovs_nsh_key_base and of > > struct nsh_hdr had the same names? > > Such change also will impact on OVS code, so I prefer not to change > them. We do not care. The order in which you send patches to different projects is your choice. The only standard by which we measure and evaluate kernel patches is the technical suitability of the patches. Whether or not some other projects have dependencies on some kind of previous versions of out of tree kernel patches have zero relevance here. If you designed other code to depend on your notion on how a kernel API will look like before getting the actual patches accepted to the kernel, that's your problem and you'll have to deal with that. > For struct nsh_hdr, we need more self-descriptive fields, but for struct > ovs_nsh_key_base, because we need to use keys in OVS OpenFlow, so np is > obviously better than next_proto, we also try our best to make sure the > old NSH implementation has same match fields as the new one does. Not relevant. Jiri
