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