[ovs-dev] [PATCH v3 06/11] Native OpenFlow 1.2 set field action.

Jarno Rajahalme jrajahalme at nicira.com
Thu Oct 24 18:40:40 UTC 2013


On Oct 23, 2013, at 9:14 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Oct 23, 2013 at 12:53:01PM -0700, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> Is MFF_VLAN_VID the only field for which the "match" and "set"
> prerequisites differ?  If so, then I'd like to think of some way to
> avoid having twice as many prerequisites that one could get wrong.
> One way is by reading the literal text of the standard.  OF1.3 says,
> "The match of the flow entry must contain the OXM prerequisite
> corresponding to the field to be set (see A.2.3.6), ...".  Referring
> to the table in A.2.3.7, we see that OXM_OF_VLAN_VID has no
> prerequisite.  So according to a strict reading of the spec we should
> not reject such a Set-Field action.

I addressed this in a separate email.

> 
> More inline:
> 
>> +/* Returns true if 'value' may be a valid value *as part of a set field
>> + * action*, false otherwise.
>> + *
>> + * A value is rejected if it is not valid for the field in question.
>> + * For example, the MFF_VLAN_TCI field will never have a nonzero value
>> + * without the VLAN_CFI bit being set, so we reject those values.
>> + * Similarly, the MFF_VLAN_VID can have the CFI bit set for matching, but
>> + * not for setting a field value, so we reject values that have the CFI bit
>> + * set.
>> + */
>> +bool
>> +mf_is_value_valid_for_set_field(const struct mf_field *mf,
>> +                                const union mf_value *value)
>> +{
> 
> I think that the following check could be implemented with fewer
> special cases via mf_read_subfield() or mf_get_subfield():

I ditched this function and open-coded it using mf_is_value_valid(). Please let me know what you think about this when I send the new patch.

>> +    /* Check that the field has no extra bits set. */
>> +    if (mf->n_bits < mf->n_bytes * 8) {
>> +        /* Check only cases for which we support. */
>> +        switch (mf->n_bytes) {
>> +        case 1:
>> +            if (value->u8 & ~0u << mf->n_bits) {
>> +                goto invalid_value;
>> +            }
>> +            break;
>> +        case 2:
>> +            if (ntohs(value->be16) & ~0u << mf->n_bits) {
>> +                goto invalid_value;
>> +            }
>> +            break;
>> +        case 4:
>> +            if (ntohl(value->be32) & ~0u << mf->n_bits) {
>> +                goto invalid_value;
>> +            }
>> +            break;
>> +        default:
>> +            goto invalid_value;
>> +        }
>> +    }
> 
> I know that it seems awkward to list many fields that don't matter,
> but I prefer to leave off the cast to int and then list them anyway,
> because it makes it easy to correctly add new fields later if one can
> say, "Add your field to the enum in meta-flow.h and then the compiler
> will tell you all the places you need to add code":
>> +    /* Check only fields for which the bit size check above is not enough. */
>> +    switch ((int)mf->id) {
>> +
> 
> The code might be easier to read (because it would be shorter) if each
> case just assigned to a variable 'valid' or 'ok' which got checked
> after the switch:
>> +    case MFF_IN_PORT_OXM: {
>> +        ofp_port_t port;
>> +        if (ofputil_port_from_ofp11(value->be32, &port)) {
>> +            goto invalid_value;
>> +        }
>> +    }
>> +        break;
>> +    case MFF_IP_DSCP: /* Low bits must be zero. */
>> +        if (value->u8 & ~IP_DSCP_MASK) {
>> +            goto invalid_value;
>> +        }
>> +        break;
>> +
>> +    case MFF_ARP_OP: /* Only really has 8 bits. */
>> +        if (value->be16 & htons(0xff00)) {
>> +            goto invalid_value;
>> +        }
>> +        break;
>> +
>> +    case MFF_VLAN_TCI: /* CFI must be set for all non-zero values. */
>> +        if (value->be16 && !(value->be16 & htons(VLAN_CFI))) {
>> +            goto invalid_value;
>> +        }
>> +        break;
>> +    }
>> +    return true;
>> +
>> + invalid_value: {
>> +        struct ds ds = DS_EMPTY_INITIALIZER;
>> +        mf_format(mf, value, NULL, &ds);
>> +        VLOG_WARN_RL(&rl, "Invalid value for destination field %s: %s",
>> +                     mf->name, ds_cstr(&ds));
>> +        ds_destroy(&ds);
>> +    }
>> +    return false;
>> +}
>> +
>> /* Returns true if 'value' may be a valid value *as part of a masked match*,
>>  * false otherwise.
>>  *
> 
> ...
> 
>> static enum ofperr
>> +set_field_from_openflow(const struct ofp12_action_set_field *oasf,
>> +                        struct ofpbuf *ofpacts)
>> +{
>> +    uint16_t oasf_len = ntohs(oasf->len);
>> +    uint32_t oxm_header = ntohl(oasf->dst);
>> +    uint8_t oxm_length = NXM_LENGTH(oxm_header);
>> +    struct ofpact_set_field *sf;
>> +    const struct mf_field *mf;
>> +
>> +    /* ofp12_action_set_field is padded to 64 bits by zero */
> 
> I generally omit parentheses around the operand of sizeof when I can
> (as one may here):

Fixed.

>> +    if (oasf_len != ROUND_UP(sizeof(*oasf) + oxm_length, 8)) {
>> +        return OFPERR_OFPBAC_BAD_SET_LEN;
>> +    }
> 
> The parentheses around 'oasf' here don't help with anything, since
> prefix operators like casts have high precedence (second only to
> postfix operators):

Fixed.

>> +    if (!is_all_zeros((const uint8_t *)(oasf) + sizeof *oasf + oxm_length,
>> +                      oasf_len - oxm_length - sizeof *oasf)) {
>> +        return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
>> +    }
>> +
>> +    if (NXM_HASMASK(oxm_header)) {
>> +        return OFPERR_OFPBAC_BAD_SET_TYPE;
>> +    }
>> +    mf = mf_from_nxm_header(oxm_header);
>> +    if (!mf) {
>> +        return OFPERR_OFPBAC_BAD_SET_TYPE;
>> +    }
>> +    ovs_assert(mf->n_bytes == oxm_length);
>> +    /* oxm_length is now validated to be compatible with mf_value. */
>> +
>> +    sf = ofpact_put_SET_FIELD(ofpacts);
>> +    sf->field = mf->id;
> 
> The value copied into sf->value should be appropriate for some member,
> but not necessarily 'u8', so I would tend to write just &sf->value
> here to avoid the impression that 'u8' is special:

Fixed, thanks.

>> +    memcpy(&sf->value.u8, oasf + 1, mf->n_bytes);
>> +
>> +    if (!mf_is_value_valid_for_set_field(mf, &sf->value)) {
>> +        return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
>> +    }
>> +    return 0;
>> +}
> 
> At least the "learn" action is also variable-length.  I am still not
> sure that open-coding it is a problem:

I removed this comment (which I inherited form the existing code).

>> +    /* Set field is the only action of variable length (so far),
>> +     * so handling the variable length portion is open-coded here */
>> +    oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
>> +    oasf->dst = htonl(mf->oxm_header);
>> +    oasf->len = htons(ntohs(oasf->len) + padded_value_len);
>> +
>> +    value = ofpbuf_put_zeros(openflow, padded_value_len);
>> +    memcpy(value, &sf->value, mf->n_bytes);
>> +}
> 
>> +    if (mf->n_bits > 64) {
>> +        ovs_assert(mf->n_bytes == 16); /* IPv6 addr. */
>> +        /* Split into 64bit chunks */
>> +        /* Lower bits first. */
>> +        narl = ofputil_put_NXAST_REG_LOAD(openflow);
>> +        narl->ofs_nbits = nxm_encode_ofs_nbits(0, 64);
>> +        narl->dst = htonl(mf->nxm_header);
> 
> This reads oddly to me.  Why this:
>> +        narl->value = *(&sf->value.be64 + 1);
> instead of just:
>           narl->value = sf->value.be64[1];
> 

sf->value is an union mf_value, not union mf_subvalue, so the be64 member is not an array. This is intentional, as the set-field action can only be used to set the whole field. I'll change this to be clearer, though.

>> +        /* Higher bits next. */
>> +        narl = ofputil_put_NXAST_REG_LOAD(openflow);
>> +        narl->ofs_nbits = nxm_encode_ofs_nbits(64, mf->n_bits - 64);
>> +        narl->dst = htonl(mf->nxm_header);
> 
> Hmm, maybe I'm missing something important (I didn't compile this
> patch) because it seems like we'd need to say be64[0] here instead of
> just be64:

Ditto.

>> +        narl->value = sf->value.be64;
>> +    } else {
>> +        narl = ofputil_put_NXAST_REG_LOAD(openflow);
>> +        narl->ofs_nbits = nxm_encode_ofs_nbits(0, mf->n_bits);
>> +        narl->dst = htonl(mf->nxm_header);
>> +        memset(&narl->value, 0, 8 - mf->n_bytes);
>> +        memcpy((char*)&narl->value + (8 - mf->n_bytes),
>> +               &sf->value, mf->n_bytes);
>> +    }
>> +}
>> +
> 
>
  Jarno





More information about the dev mailing list