Ben,
OpenFlow 1.3.3 is much more specific about handling the VID with
set-field action. It states that the OFPVID_PRESENT bit must be on,
and that the switch behavior when VLAN header does not exist is
implementation-dependent:
"The use of a set-fi eld action assumes that the corresponding
header exist in the packet, the e ffect of a set-fi eld action on a
packet that does not have the corresponding header fi eld is
undefi ned (switch and fi eld dependant). In particular, when
setting the VID on a packet without a VLAN header, a switch
may or may not automatically add a new VLAN tag, and the
controller must explicitly use a Push VLAN header
action to be compatible with all switch implementations."
IMO we should do nothing if the field does not exist already, even though
this is different from what we have done before with OF 1.0. It should also
be noted that our existing set-field implementation with internal reg_load
action NEVER set the CFI bit, as we have had n_bits as 12 for MFF_VLAN_VID.
Similar consideration applies for the MPLS label and tc: When setting these
fields, they should only ever apply to an existing MPLS header.
How to handle prerequisites is another issue. True, the table does not
require existing VLAN header for matching, which is sensible since
matching is explicitly possible for non-present headers. However, the
above text makes it clear that set-field should be performed on
existing headers only, so it would be prudent to flag a missing VLAN
header as an inconsistency for a set-field on VLAN VID.
Jarno
On Oct 23, 2013, at 9:14 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Wed, Oct 23, 2013 at 12:53:01PM -0700, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme <jrajaha...@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);
>> + }
>> +}
>> +
>
> ...
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
