All of those seem fine to me. I like getting rid of the extra "hdr=nsh". I think that this should be much more usable and readable.
On Wed, Aug 02, 2017 at 01:04:39AM +0000, Yang, Yi Y wrote: > Maybe > encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)) > is better, the below are possible formats, I think they are all ok, what do > you think about them? > > > encap(nsh) > encap(nsh()) > encap(nsh(md_type=1)) > encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)) > > -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Wednesday, August 2, 2017 7:17 AM > To: Yang, Yi Y <[email protected]> > Cc: [email protected]; Jan Scheurich <[email protected]>; Zoltan > Balogh <[email protected]> > Subject: Re: [PATCH v4 1/2] OF support and translation of generic encap and > decap > > encap(hdr=nsh,nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)) > > On Tue, Aug 01, 2017 at 10:53:27PM +0000, Yang, Yi Y wrote: > > Ben, because we're considering to cover NSH md type 2 case, for NSH TLV, > > now we provide it by the below way. > > > > encap(hdr=nsh,prop(class=nsh,type=md_type,val=2),prop(class=nsh,type=t > > lv,val(0x1000,10,0x12345678)),prop(class=nsh,type=tlv,val(0x2000,20,0x > > fedcba9876543210))) > > > > Can you help provide a format you prefer to handle this use case? > > > > -----Original Message----- > > From: Ben Pfaff [mailto:[email protected]] > > Sent: Tuesday, August 1, 2017 11:56 PM > > To: Yang, Yi Y <[email protected]> > > Cc: [email protected]; Jan Scheurich <[email protected]>; > > Zoltan Balogh <[email protected]> > > Subject: Re: [PATCH v4 1/2] OF support and translation of generic > > encap and decap > > > > On Tue, Aug 01, 2017 at 08:06:59PM +0800, Yi Yang wrote: > > > From: Jan Scheurich <[email protected]> > > > > > > This commit adds support for the OpenFlow actions generic encap and > > > decap (as specified in ONF EXT-382) to the OVS control plane. > > > > > > CLI syntax for encap action with properties: > > > encap(hdr=<header>) > > > encap(hdr=<header>, > > > prop(class=<class>,type=<type>,val=<simple>), > > > prop(class=<class>,type=<type>,val(<complex>))) > > > > > > CLI syntax for decap action: > > > decap() > > > decap(packet_type(ns=<pt_ns>,type=<pt_type>)) > > > > > > The first header supported for encap and decap is "ethernet" to > > > convert packets between packet_type (1,Ethertype) and (0,0). > > > > > > This commit also implements a skeleton for the translation of > > > generic encap and decap actions in ofproto-dpif and adds support to > > > encap and decap an Ethernet header. > > > > > > In general translation of encap commits pending actions and then > > > rewrites struct flow in accordance with the new packet type and > > > header. In the case of encap(ethernet) it suffices to change the > > > packet type from (1, Ethertype) to (0,0) and set the dl_type > > > accordingly. A new pending_encap flag in xlate ctx is set to mark > > > that an corresponding datapath encap action must be triggered at the > > > next commit. In the case of encap(ethernet) ofproto generetas a push_eth > > > action. > > > > > > The general case for translation of decap() is to emit a datapath > > > action to decap the current outermost header and then recirculate > > > the packet to reparse the inner headers. In the special case of an > > > Ethernet packet, > > > decap() just changes the packet type from (0,0) to (1, dl_type) > > > without a need to recirculate. The emission of the pop_eth action > > > for the datapath is postponed to the next commit. > > > > > > Hence encap(ethernet) and decap() on an Ethernet packet are OF > > > octions that only incur a cost in the dataplane when a modifed > > > packet is actually committed, e.g. because it is sent out. They can > > > freely be used for normalizing the packet type in the OF pipeline > > > without degrading performance. > > > > > > Signed-off-by: Jan Scheurich <[email protected]> > > > Signed-off-by: Yi Yang <[email protected]> > > > Signed-off-by: Zoltan Balogh <[email protected]> > > > Co-authored-by: Zoltan Balogh <[email protected]> > > > > Thanks for iterating so quickly! > > > > Besides the syntax for properties, which I still think should be simplified > > to e.g. nsh(md_type=1), I have only a few remaining comments. > > > > I don't think there's any value in the n_props member in nx_action_encap. > > (This is about the OpenFlow wire format now, not the internal format.) The > > properties are the whole tail of the structure and having a count doesn't > > make anything easier. Can you remove it? It will allow us to drop 8 bytes > > from the action structure due to padding. > > (In case it can't be removed, I'm providing a spelling fix.) > > > > decode_ed_prop() still doesn't check the length properly, so I'm providing > > a fix. > > > > I'm appending my suggested incremental. > > > > Thanks again! > > > > --8<--------------------------cut here-------------------------->8-- > > > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index > > d0437f20922a..c7d47eb5dd71 100644 > > --- a/lib/ofp-actions.c > > +++ b/lib/ofp-actions.c > > @@ -4036,7 +4036,7 @@ struct nx_action_encap { > > ovs_be16 subtype; /* NXAST_ENCAP. */ > > ovs_be16 hdr_size; /* Header size in bytes, 0 = 'not specified'.*/ > > ovs_be32 new_pkt_type; /* Header type to add and PACKET_TYPE of > > result. */ > > - ovs_be16 n_props; /* Number of the following endecap properties. > > */ > > + ovs_be16 n_props; /* Number of the following encap properties. */ > > uint8_t pad[6]; /* Align to 8 bytes boundary */ > > struct ofp_ed_prop_header props[]; /* Encap TLV properties. */ }; @@ > > -4263,6 +4263,11 @@ decode_NXAST_RAW_DECAP(const struct nx_action_decap > > *nad, > > enum ofp_version ofp_version OVS_UNUSED, > > struct ofpbuf *ofpacts) { > > + if (ntohs(nad->len) > sizeof *nad) { > > + /* No properties supported yet. */ > > + return OFPERR_OFPBPC_BAD_TYPE; > > + } > > + > > struct ofpact_decap * decap; > > > > decap = ofpact_put_DECAP(ofpacts); diff --git > > a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index > > 260adc30acf0..6bba3ac7d982 100644 > > --- a/lib/ofp-ed-props.c > > +++ b/lib/ofp-ed-props.c > > @@ -32,6 +32,9 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, > > uint16_t prop_class = ntohs((*ofp_prop)->prop_class); > > size_t len = (*ofp_prop)->len; > > size_t pad_len = ROUND_UP(len, 8); > > + if (pad_len > *remaining) { > > + return OFPERR_OFPBAC_BAD_LEN; > > + } > > > > switch (prop_class) { > > default: _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
