On Mon, Oct 06, 2014 at 11:26:09AM -0700, Jarno Rajahalme wrote:
>
> On Sep 30, 2014, at 5:47 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > OpenFlow 1.5 (draft) extends the OFPAT_SET_FIELD action originally
> > introduced in OpenFlow 1.2 so that it can set not just entire fields but
> > any subset of bits within a field as well. This commit adds support for
> > that feature when OpenFlow 1.5 is used.
> >
> > With this feature, OFPAT_SET_FIELD becomes a superset of NXAST_REG_LOAD.
> > Thus, this commit merges the implementations of the two actions into a
> > single ofpact_set_field.
> >
> > ONF-JIRA: EXT-314
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ?
> >
>
> (snip)
>
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index f6818ca..ca4dac9 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
>
> (snip)
>
> > @@ -1973,14 +1932,17 @@ decode_OFPAT_RAW12_SET_FIELD(const struct
> > ofp12_action_set_field *oasf,
> > sf = ofpact_put_SET_FIELD(ofpacts);
> >
> > ofpbuf_use_const(&b, oasf, ntohs(oasf->len));
> > - ofpbuf_pull(&b, 4);
> > - error = nx_pull_entry(&b, &sf->field, &sf->value, NULL);
> > + ofpbuf_pull(&b, OBJECT_OFFSETOF(oasf, pad));
> > + error = nx_pull_entry(&b, &sf->field, &sf->value,
> > + may_mask ? &sf->mask : NULL);
> > if (error) {
> > - /* OF1.5 specifically says to use OFPBAC_BAD_SET_MASK in this
> > case. */
> > return (error == OFPERR_OFPBMC_BAD_MASK
> > ? OFPERR_OFPBAC_BAD_SET_MASK
> > : error);
> > }
> > + if (!may_mask) {
> > + memset(&sf->mask, 0xff, sf->field->n_bytes);
> > + }
> >
> > if (!is_all_zeros(ofpbuf_data(&b), ofpbuf_size(&b))) {
> > return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> > @@ -2003,6 +1965,7 @@ decode_OFPAT_RAW12_SET_FIELD(const struct
> > ofp12_action_set_field *oasf,
> > * on for OXM_OF_VLAN_VID. */
> > if (!mf_is_value_valid(sf->field, &sf->value)
> > || (sf->field->id == MFF_VLAN_VID
> > + && sf->mask.be16 & htons(OFPVID12_PRESENT)
> > && !(sf->value.be16 & htons(OFPVID12_PRESENT)))) {
>
> This does not look right to me.
It was a little wrong. Like this, then?
/* The value must be valid for match. The OpenFlow 1.5 draft also says,
* "In an OXM_OF_VLAN_VID set-field action, the OFPVID_PRESENT bit must be
* a 1-bit in oxm_value and in oxm_mask." */
if (!mf_is_value_valid(sf->field, &sf->value)
|| (sf->field->id == MFF_VLAN_VID
&& (!(sf->mask.be16 & htons(OFPVID12_PRESENT)
|| !(sf->value.be16 & htons(OFPVID12_PRESENT)))))) {
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
