[ovs-dev] [PATCH v2 14/16] ofp-actions: Centralize all OpenFlow action code for maintainability.
Jarno Rajahalme
jrajahalme at nicira.com
Mon Aug 11 18:34:58 UTC 2014
Nice cleanup, especially like the way wire formats are now hidden inside lib/ofp-actions.c.
Some small comments below,
Jarno
Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
On Aug 7, 2014, at 4:14 PM, Ben Pfaff <blp at nicira.com> wrote:
> Until now, knowledge about OpenFlow has been somewhat scattered around the
> tree. Some of it is in ofp-actions, some of it is in ofp-util, some in
> separate files for individual actions, and most of the wire format
> declarations are in include/openflow. This commit centralizes all of that
> in ofp-actions.
>
> Encoding and decoding OpenFlow actions was previously broken up by OpenFlow
> version. This was OK with only OpenFlow 1.0 and 1.1, but each additional
> version added a new wrapper around the existing ones, which started to
> become hard to understand. This commit merges all of the processing for
> the different versions, to the extent that they are similar, making the
> version differences clearer.
>
> Previously, ofp-actions contained OpenFlow encoding and decoding, plus
> ofpact formatting, but OpenFlow parsing was separated into ofp-parse, which
> seems an odd division. This commit moves the parsing code into ofp-actions
> with the rest of the code.
>
> Before this commit, the four main bits of code associated with a particular
> ofpact--OpenFlow encoding and decoding, ofpact formatting and parsing--were
> all found far away from each other. This often made it hard to see what
> was going on for a particular ofpact, since you had to search around to
> many different pieces of code. This commit reorganizes so that all of the
> code for a given ofpact is in a single place.
>
> As a code refactoring, this commit has little visible behavioral change.
> The update to ofproto-dpif.at illustrates one minor bug fix as a side
> effect: a flow that was added with the action "dec_ttl" (a standard
> OpenFlow action) was previously formatted as "dec_ttl(0)" (using a Nicira
> extension to specifically direct packets bounced to the controller because
> of too-low TTL), but after this commit it is correctly formatted as
> "dec_ttl".
>
> The other visible effect is to drop support for the Nicira extension
> dec_ttl action in OpenFlow 1.1 and later in favor of the equivalent
> standard action. It seems unlikely that anyone was really using the
> Nicira extension in OF1.1 or later.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> build-aux/extract-ofp-actions | 376 ++
> include/openflow/nicira-ext.h | 1014 -----
> include/openflow/openflow-1.0.h | 50 +-
> include/openflow/openflow-1.1.h | 123 -
> include/openflow/openflow-1.2.h | 20 -
> include/openflow/openflow-1.3.h | 21 -
> include/openflow/openflow-common.h | 76 -
> lib/automake.mk | 8 +-
> lib/bundle.c | 115 +-
> lib/bundle.h | 7 +-
> lib/learn.c | 212 -
> lib/learn.h | 6 +-
> lib/multipath.c | 60 +-
> lib/multipath.h | 11 +-
> lib/nx-match.c | 121 -
> lib/nx-match.h | 19 -
> lib/ofp-actions.c | 8045 +++++++++++++++++++++++-------------
> lib/ofp-actions.h | 150 +-
> lib/ofp-msgs.h | 2 +-
> lib/ofp-parse.c | 1104 +----
> lib/ofp-parse.h | 4 -
> lib/ofp-util.c | 115 -
> lib/ofp-util.def | 104 -
> lib/ofp-util.h | 93 -
> ofproto/ofproto-dpif.c | 3 +-
> ofproto/ofproto.c | 2 +-
> tests/ofp-actions.at | 4 -
> tests/ofproto-dpif.at | 8 +-
> utilities/ovs-ofctl.c | 2 +-
> 29 files changed, 5743 insertions(+), 6132 deletions(-)
> create mode 100755 build-aux/extract-ofp-actions
> delete mode 100644 lib/ofp-util.def
>
>
(snip)
> +/* Parses 'str' as a series of instructions, and appends them to 'ofpacts'.
> + *
> + * Returns NULL if successful, otherwise a malloc()'d string describing the
> + * error. The caller is responsible for freeing the returned string. */
> +static char * WARN_UNUSED_RESULT
> +ofpacts_parse__(char *str, struct ofpbuf *ofpacts,
> + enum ofputil_protocol *usable_protocols,
> + bool allow_instructions)
> +{
> + int prev_inst = -1;
> + enum ofperr retval;
> + char *key, *value;
> + bool drop = false;
> + char *pos;
> +
> + pos = str;
> + while (ofputil_parse_key_value(&pos, &key, &value)) {
> + enum ovs_instruction_type inst = OVSINST_OFPIT11_APPLY_ACTIONS;
> + enum ofpact_type type;
> + char *error = NULL;
> + ofp_port_t port;
> +
> + if (ofpact_type_from_name(key, &type)) {
> + error = ofpact_parse(type, value, ofpacts, usable_protocols);
> + inst = ovs_instruction_type_from_ofpact_type(type);
> + } else if (!strcasecmp(key, "mod_vlan_vid")) {
> + error = parse_set_vlan_vid(value, ofpacts, true);
> + } else if (!strcasecmp(key, "mod_vlan_pcp")) {
> + error = parse_set_vlan_pcp(value, ofpacts, true);
> + } else if (!strcasecmp(key, "set_nw_ttl")) {
> + error = parse_SET_IP_TTL(value, ofpacts, usable_protocols);
> + } else if (!strcasecmp(key, "pop_vlan")) {
> + error = parse_pop_vlan(ofpacts);
> + } else if (!strcasecmp(key, "set_tunnel64")) {
> + error = parse_set_tunnel(value, ofpacts,
> + NXAST_RAW_SET_TUNNEL64);
> + } else if (!strcasecmp(key, "bundle_load")) {
> + error = parse_bundle_load(value, ofpacts);
> + } else if (!strcasecmp(key, "drop")) {
> + drop = true;
> + } else if (!strcasecmp(key, "apply_actions")) {
> + return xstrdup("apply_actions is the default instruction");
> + } else if (ofputil_port_from_string(key, &port)) {
> + ofpact_put_OUTPUT(ofpacts)->port = port;
> + } else {
> + return xasprintf("unknown action %s", key);
> + }
> + if (error) {
> + return error;
> + }
> +
> + if (inst != OVSINST_OFPIT11_APPLY_ACTIONS) {
> + if (!allow_instructions) {
> + return xasprintf("only actions are allowed here (not "
> + "instruction %s)",
> + ovs_instruction_name_from_type(inst));
> + }
> + if (inst == prev_inst) {
> + return xasprintf("instruction %s may be specified only once",
> + ovs_instruction_name_from_type(inst));
> + }
> + }
> + if (prev_inst != -1 && inst < prev_inst) {
> + return xasprintf("instruction %s must be specified before %s",
> + ovs_instruction_name_from_type(inst),
> + ovs_instruction_name_from_type(prev_inst));
> + }
> + prev_inst = inst;
> + }
> + ofpact_pad(ofpacts);
> +
> + if (drop && ofpbuf_size(ofpacts)) {
> + return xstrdup("\"drop\" must not be accompanied by any other action "
> + "or instruction");
> + }
> +
> + /* XXX
> + *
> + * If write_metadata is specified as an action AND an instruction, ofpacts
> + * could be invalid. */
Could this be resolved by not allowing the action for OpenFlow versions which support the instruction?
> + retval = ofpacts_verify(ofpbuf_data(ofpacts), ofpbuf_size(ofpacts),
> + (allow_instructions
> + ? (1u << N_OVS_INSTRUCTIONS) - 1
> + : 1u << OVSINST_OFPIT11_APPLY_ACTIONS));
> + if (retval) {
> + return xstrdup("Incorrect instruction ordering");
> + }
> +
> + return NULL;
> +}
> +
(snip)
> @@ -2096,53 +1086,59 @@ static char * WARN_UNUSED_RESULT
> parse_bucket_str(struct ofputil_bucket *bucket, char *str_,
> enum ofputil_protocol *usable_protocols)
> {
> + char *pos, *key, *value;
> struct ofpbuf ofpacts;
> - char *pos, *act, *arg;
> - int n_actions;
> + struct ds actions;
> + char *error;
>
> bucket->weight = 1;
> bucket->watch_port = OFPP_ANY;
> bucket->watch_group = OFPG11_ANY;
>
> - pos = str_;
> - n_actions = 0;
> - ofpbuf_init(&ofpacts, 64);
> - while (ofputil_parse_key_value(&pos, &act, &arg)) {
> - char *error = NULL;
> + ds_init(&actions);
>
> - if (!strcasecmp(act, "weight")) {
> - error = str_to_u16(arg, "weight", &bucket->weight);
> - } else if (!strcasecmp(act, "watch_port")) {
> - if (!ofputil_port_from_string(arg, &bucket->watch_port)
> + pos = str_;
> + error = NULL;
> + while (ofputil_parse_key_value(&pos, &key, &value)) {
> + if (!strcasecmp(key, "weight")) {
> + error = str_to_u16(value, "weight", &bucket->weight);
> + } else if (!strcasecmp(key, "watch_port")) {
> + if (!ofputil_port_from_string(value, &bucket->watch_port)
> || (ofp_to_u16(bucket->watch_port) >= ofp_to_u16(OFPP_MAX)
> && bucket->watch_port != OFPP_ANY)) {
> - error = xasprintf("%s: invalid watch_port", arg);
> + error = xasprintf("%s: invalid watch_port", value);
> }
> - } else if (!strcasecmp(act, "watch_group")) {
> - error = str_to_u32(arg, &bucket->watch_group);
> + } else if (!strcasecmp(key, "watch_group")) {
> + error = str_to_u32(value, &bucket->watch_group);
> if (!error && bucket->watch_group > OFPG_MAX) {
> error = xasprintf("invalid watch_group id %"PRIu32,
> bucket->watch_group);
> }
> - } else if (!strcasecmp(act, "actions")) {
> - if (ofputil_parse_key_value(&arg, &act, &arg)) {
> - error = str_to_ofpact__(pos, act, arg, &ofpacts, n_actions,
> - usable_protocols);
> - n_actions++;
> - }
> + } else if (!strcasecmp(key, "action") || !strcasecmp(key, "actions")) {
> + ds_put_format(&actions, "%s,", value);
> } else {
> - error = str_to_ofpact__(pos, act, arg, &ofpacts, n_actions,
> - usable_protocols);
> - n_actions++;
> + ds_put_format(&actions, "%s(%s),", key, value);
> }
>
> if (error) {
> - ofpbuf_uninit(&ofpacts);
> + ds_destroy(&actions);
> return error;
> }
> }
>
> - ofpact_pad(&ofpacts);
> + if (!actions.length) {
> + return xstrdup("bucket must specify actions");
> + }
> + ds_chomp(&actions, ',');
> +
> + ofpbuf_init(&ofpacts, 0);
> + error = ofpacts_parse_actions(ds_cstr(&actions), &ofpacts,
> + usable_protocols);
> + ds_destroy(&actions);
> + if (error) {
> + ofpbuf_uninit(&ofpacts);
> + return error;
> + }
> bucket->ofpacts = ofpbuf_data(&ofpacts);
> bucket->ofpacts_len = ofpbuf_size(&ofpacts);
>
Should bucket parsing go to ofp-actions.c as well?
(snip)
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 7bf53a4..e17377f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1246,7 +1246,6 @@ add_internal_flows(struct ofproto_dpif *ofproto)
> * (priority=2), recirc=0, actions=resubmit(, 0)
> */
> resubmit = ofpact_put_RESUBMIT(&ofpacts);
> - resubmit->ofpact.compat = 0;
Doesn’t this leave the renamed ‘raw’ member as ‘-1’? Does this matter?
> resubmit->in_port = OFPP_IN_PORT;
> resubmit->table_id = 0;
(snip)
More information about the dev
mailing list