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;
}
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
