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

Ben Pfaff blp at nicira.com
Thu Oct 24 04:14:10 UTC 2013


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.

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():
> +    /* 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):
> +    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):
> +    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:
> +    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:
> +    /* 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];

> +        /* 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:
> +        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);
> +    }
> +}
> +

...



More information about the dev mailing list