On Fri, Nov 01, 2013 at 03:22:34PM -0700, Jarno Rajahalme wrote:
> Fair enough ;-)
Thanks.
I'm folding in the following additional changes, and then I'll apply
this to master.
The only really notable part of the changes that I made is that I'm
feeling timid about silently adding and removing the OFPVID_PRESENT
bit. It will be a convenience to some users, but I think that it will
be confusing to others, especially to anyone who thinks that the
string format of actions accurately represents what is in the binary
form. On that balance, at the moment I prefer to do what requires
fewer special cases, so I've removed the code marked RFC. If you feel
differently, then let's talk about it.
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 9f31449..761cea5 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -788,8 +788,7 @@ set_field_from_openflow(const struct ofp12_action_set_field
*oasf,
ovs_assert(mf->n_bytes == oxm_length);
/* oxm_length is now validated to be compatible with mf_value. */
if (!mf->writable) {
- VLOG_WARN_RL(&rl, "destination field %s is not writable",
- mf->name);
+ VLOG_WARN_RL(&rl, "destination field %s is not writable", mf->name);
return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
}
sf = ofpact_put_SET_FIELD(ofpacts);
@@ -813,8 +812,8 @@ set_field_from_openflow(const struct ofp12_action_set_field
*oasf,
}
static void
-set_field_to_ofast(const struct ofpact_set_field *sf,
- struct ofpbuf *openflow)
+set_field_to_openflow12(const struct ofpact_set_field *sf,
+ struct ofpbuf *openflow)
{
uint16_t padded_value_len = ROUND_UP(sf->field->n_bytes, 8);
struct ofp12_action_set_field *oasf;
@@ -822,27 +821,19 @@ set_field_to_ofast(const struct ofpact_set_field *sf,
oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
oasf->dst = htonl(sf->field->oxm_header);
- oasf->len = htons(ntohs(oasf->len) + padded_value_len);
+ oasf->len = htons(sizeof *oasf + padded_value_len);
value = ofpbuf_put_zeros(openflow, padded_value_len);
memcpy(value, &sf->value, sf->field->n_bytes);
}
+/* Convert 'sf' to one or two REG_LOADs. */
static void
-set_field_to_openflow(const struct ofpact_set_field *sf,
- struct ofpbuf *openflow)
+set_field_to_nxast(const struct ofpact_set_field *sf, struct ofpbuf *openflow)
{
- struct ofp_header *oh = (struct ofp_header *)openflow->l2;
const struct mf_field *mf = sf->field;
struct nx_action_reg_load *narl;
- if (oh->version >= OFP12_VERSION) {
- set_field_to_ofast(sf, openflow);
- return;
- }
-
- /* Convert to one or two REG_LOADs */
-
if (mf->n_bits > 64) {
ovs_assert(mf->n_bytes == 16); /* IPv6 addr. */
/* Split into 64bit chunks */
@@ -866,6 +857,19 @@ set_field_to_openflow(const struct ofpact_set_field *sf,
}
}
+static void
+set_field_to_openflow(const struct ofpact_set_field *sf,
+ struct ofpbuf *openflow)
+{
+ struct ofp_header *oh = (struct ofp_header *)openflow->l2;
+
+ if (oh->version >= OFP12_VERSION) {
+ set_field_to_openflow12(sf, openflow);
+ } else {
+ set_field_to_nxast(sf, openflow);
+ }
+}
+
static enum ofperr
output_from_openflow11(const struct ofp11_action_output *oao,
struct ofpbuf *out)
@@ -2827,8 +2831,6 @@ ofpact_format(const struct ofpact *a, struct ds *s)
const struct ofpact_sample *sample;
const struct ofpact_set_field *set_field;
const struct mf_field *mf;
- const union mf_value *value;
- union mf_value temp;
ofp_port_t port;
switch (a->type) {
@@ -2966,14 +2968,7 @@ ofpact_format(const struct ofpact *a, struct ds *s)
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
ds_put_format(s, "set_field:");
- /* RFC: Print VLAN VID without the OFPVID_PRESENT bit. */
- if (mf->id == MFF_VLAN_VID) {
- temp.be16 = set_field->value.be16 & htons(VLAN_VID_MASK);
- value = &temp;
- } else {
- value = &set_field->value;
- }
- mf_format(mf, value, NULL, s);
+ mf_format(mf, &set_field->value, NULL, s);
ds_put_format(s, "->%s", mf->name);
break;
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 233a31b..0478a9b 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -379,7 +379,7 @@ enum ofpact_mpls_position {
struct ofpact_set_field {
struct ofpact ofpact;
const struct mf_field *field;
- union mf_value value; /* Most-significant bits are used. */
+ union mf_value value;
};
/* OFPACT_PUSH_VLAN/MPLS/PBB
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 0fb32d5..83413c2 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -499,10 +499,6 @@ set_field_parse__(char *arg, struct ofpbuf *ofpacts,
if (!mf_is_value_valid(mf, &sf->value)) {
return xasprintf("%s is not a valid value for field %s", value, key);
}
- /* RFC: Silently add the required OFPVID_PRESENT bit. */
- if (mf->id == MFF_VLAN_VID) {
- sf->value.be16 |= htons(OFPVID12_PRESENT);
- }
*usable_protocols &= mf->usable_protocols;
return NULL;
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
