[ovs-dev] [PATCH v2 06/12] ofp-actions: Support OF1.5 (draft) masked Set-Field, merge with reg_load.

Jarno Rajahalme jrajahalme at nicira.com
Wed Oct 8 23:14:32 UTC 2014


On Oct 7, 2014, at 4:31 PM, Ben Pfaff <blp at 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 <blp at 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 <blp at 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


More information about the dev mailing list