On Wed, Oct 08, 2014 at 04:14:32PM -0700, Jarno Rajahalme wrote:
>
> On Oct 7, 2014, at 4:31 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > 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)))))) {
>
>
> I think in above there is one closing parenthesis in wrong place.
>
> How about this (MFF_VLAN_VID is wrong if either of the PRESENT bits is zero):
>
> /* 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))))) {
> struct ds ds = DS_EMPTY_INITIALIZER;
> mf_format(sf->field, &sf->value, NULL, &ds);
> VLOG_WARN_RL(&rl, "Invalid value for set field %s: %s",
> sf->field->name, ds_cstr(&ds));
> ds_destroy(&ds);
>
> return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> }
You are right. I agree.
I will apply the following in a minute:
--8<--------------------------cut here-------------------------->8--
Subject: [PATCH] ofp-actions: Correct test for OFPVID_PRESENT bit in
set_field action.
Reported-by: Jarno Rajahalme <jrajaha...@nicira.com>
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
lib/ofp-actions.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index de46e08..f5169ff 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1966,8 +1966,8 @@ decode_ofpat_set_field(const struct
ofp12_action_set_field *oasf,
* 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)))))) {
+ && (!(sf->mask.be16 & htons(OFPVID12_PRESENT))
+ || !(sf->value.be16 & htons(OFPVID12_PRESENT))))) {
struct ds ds = DS_EMPTY_INITIALIZER;
mf_format(sf->field, &sf->value, NULL, &ds);
VLOG_WARN_RL(&rl, "Invalid value for set field %s: %s",
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
