On Tue, Aug 01, 2017 at 12:04:55AM +0000, Yang, Yi Y wrote:
> Hi, Ben
>
> Thank you so much for your great review and great comments, I'll do changes
> per your comments and post next version because Zoltan and Jan are taking
> vacation. I explained your concerns
>
> [Ben] The string format of the encap actions seems rather user unfriendly.
> Is there a good reason why it should be the very generic
> prop(class=<class>,type=<type>,val=<val>) rather than something more tailored
> to the actual properties that users will want to set? It is hard to tell
> without any actual examples.
>
> [Yi] because encap is very generic action, you can add a new header/encap
> type and some new properties for it very easily by this kind of very generic
> abstraction and mustn't change generic encap implementation. I make an
> example, for NSH we have
>
> prop(class=nsh,type=md_type,val=1)
>
> If we add new encap/header type gtpu, we can have the below property for GTP-u
>
> prop(class=gtpu,type=teid,val=1234)
>
> Generic encap action can accept this kind of new prop without any change
> against generic encap action implementation, this is the magic of very
> generic " prop(class=<class>,type=<type>,val=<val>)".
To me, this argument would make sense if it meant that we could add gtpu
without having to modify OVS. But we'll still need to do that. At the
very least, OVS isn't going to know class "gtpu" or type "teid" means
without modifications.
Here's a more user friendly syntax that seems just as generic to me:
class(type=value)
e.g.:
nsh(md_type=1)
gtpu(teid=1234)
> [Ben] In decode_NXAST_RAW_ENCAP() and parse_ENCAP(), shouldn't there be
> validation of the header size value? Most header types will only have a few
> acceptable header sizes.
>
> [Yi] Actually header_size isn't used at all now, it is reserved for later
> enhancement. For NSH MD type 2, NSH header is length-variable, ranged from 8
> to 264, number of props and size of each prop are also very flexible (like
> Geneve tunnel metadata)
OK.
> [Ben] The internal representation of ofpact_ed_prop, has no examples so far.
> How confident are you that it actually needs to be variable-length? If it
> does, then using a member 'n_props' in struct ofpact_encap to count the
> number of properties seems risky: it implicitly encourages programmers to try
> to index the props[] array from 0 to n_props-1. I'd encourage, in that case,
> switching to a length member, or just eliminating the member if
> ofpact_ed_props can be padded to 8-byte multiples. But it's easier if we can
> just make ofpact_ed_prop fixed-length for now; then we can just index props[]
> as an array.
>
> However, I question whether the properties actually need to be internally
> represented as properties at all. Presumably, OVS is only going to support a
> specific set of properties. Probably, it's easy to just add specific members
> to struct ofpact_encap that represent the values of those properties. Did
> you consider that? That approach usually simplifies code greatly; properties
> are usually needed only for external representations.
>
> [Yi] As I said above, prop is length-variable and unpredictable, especially
> for NSH MD type 2, so a length-fixed ofpact_ed_prop impracticable and will
> waste too much space, n_props can indicate number of props, actually props is
> a pointer, we can use
>
> uint8 props[]
>
> to replace
>
> struct ofp_ed_prop_header props[];
>
> But I think it doesn't bring any substantial benefit from my perspective.
>
> ofpact_ed_prop is header of each peoperty, it just helps parse properties
> better.
>
> In addition, code is completely controllable, abuse of props[index] won't
> happen because you guys check every commit very carefully :-)
OK. I still think this seems to add more complexity than is probably
needed, but I'll leave it alone for now.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
