Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
Three minor comments below,
Jarno
On Aug 4, 2014, at 9:21 AM, Ben Pfaff <b...@nicira.com> wrote:
> Until now, sets of actions have been abstracted separately outside
> ofp-actions, as enum ofputil_action_bitmap. Drawing sets of actions into
> ofp-actions, as done in this commit, makes for a better overall
> abstraction of actions, with better consistency.
>
> A big part of this commit is shifting from using ofp12_table_stats as if
> it were an abstraction for OpenFlow table stats, toward using a new
> struct ofputil_table_stats, which is what we generally do with other
> OpenFlow structures and fits better with the rest of the code.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/ofp-actions.c | 156 +++++++++++++++++++++++++
> lib/ofp-actions.h | 145 +++++++++++------------
> lib/ofp-print.c | 105 ++++-------------
> lib/ofp-util.c | 280 ++++++++++++++++++++++-----------------------
> lib/ofp-util.h | 66 ++++-------
> ofproto/ofproto-dpif.c | 23 +---
> ofproto/ofproto-provider.h | 22 ++--
> ofproto/ofproto.c | 85 ++++++--------
> tests/ofp-print.at | 26 +++--
> tests/ofproto.at | 20 ++--
> 10 files changed, 495 insertions(+), 433 deletions(-)
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index e8fd922..3818b2d 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -3091,6 +3091,151 @@ ofpacts_put_openflow_instructions(const struct ofpact
> ofpacts[],
> }
> }
>
> +/* Sets of supported actions. */
> +
> +struct ofpact_xlate {
> + enum ofpact_type ofpact;
> + int ofpat;
This would benefit from some comments:
- ‘ofpact’ OVS internal enumeration of all supported action types.
- ‘ofpat' the corresponding action number specified in a given version of the
OpenFlow specification.
Also, this has nothing to do with ofproto-dpif-xlate, but “xlate” made me
initially think this would be related.
> +};
> +
> +static const struct ofpact_xlate *
> +get_ofpact_xlate(enum ofp_version version)
> +{
> + /* OpenFlow 1.0 actions. */
> + static const struct ofpact_xlate of10[] = {
> + { OFPACT_OUTPUT, 0 },
> + { OFPACT_SET_VLAN_VID, 1 },
> + { OFPACT_SET_VLAN_PCP, 2 },
> + { OFPACT_STRIP_VLAN, 3 },
> + { OFPACT_SET_ETH_SRC, 4 },
> + { OFPACT_SET_ETH_DST, 5 },
> + { OFPACT_SET_IPV4_SRC, 6 },
> + { OFPACT_SET_IPV4_DST, 7 },
> + { OFPACT_SET_IP_DSCP, 8 },
> + { OFPACT_SET_L4_SRC_PORT, 9 },
> + { OFPACT_SET_L4_DST_PORT, 10 },
> + { OFPACT_ENQUEUE, 11 },
> + { 0, -1 },
> + };
> +
(snip)
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index 8cb8862..5877151 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -31,7 +31,7 @@
> * This macro is used directly only internally by this header, but the list is
> * still of interest to developers.
> *
> - * Each DEFINE_OFPACT invocation has the following parameters:
> + * Each OFPACT invocation has the following parameters:
> *
> * 1. <ENUM>, used below in the enum definition of OFPACT_<ENUM>, and
> * elsewhere.
Add: 4. string name of the action.
(snip)
> +static const char *
> +group_type_to_string(enum ofp11_group_type type)
> +{
> + switch (type) {
> + case OFPGT11_ALL: return "all";
> + case OFPGT11_SELECT: return "select";
> + case OFPGT11_INDIRECT: return "indirect";
> + case OFPGT11_FF: return "fast failover";
> + default: OVS_NOT_REACHED();
> + }
> +}
> +
> static void
> ofp_print_group_features(struct ds *string, const struct ofp_header *oh)
> {
> struct ofputil_group_features features;
> + int i;
>
> ofputil_decode_group_features_reply(oh, &features);
>
> @@ -2470,32 +2443,15 @@ ofp_print_group_features(struct ds *string, const
> struct ofp_header *oh)
> ds_put_format(string, " Capabilities: 0x%"PRIx32"\n",
> features.capabilities);
>
> - if (features.types & (1u << OFPGT11_ALL)) {
> - ds_put_format(string, " All group :\n");
> - ds_put_format(string,
> - " max_groups = %#"PRIx32"
> actions=0x%08"PRIx32"\n",
> - features.max_groups[0], features.actions[0]);
> - }
> -
> - if (features.types & (1u << OFPGT11_SELECT)) {
> - ds_put_format(string, " Select group :\n");
> - ds_put_format(string, " max_groups = %#"PRIx32" "
> - "actions=0x%08"PRIx32"\n",
> - features.max_groups[1], features.actions[1]);
> - }
> -
> - if (features.types & (1u << OFPGT11_INDIRECT)) {
> - ds_put_format(string, " Indirect group :\n");
> - ds_put_format(string, " max_groups = %#"PRIx32" "
> - "actions=0x%08"PRIx32"\n",
> - features.max_groups[2], features.actions[2]);
> - }
> -
> - if (features.types & (1u << OFPGT11_FF)) {
> - ds_put_format(string, " Fast Failover group :\n");
> - ds_put_format(string, " max_groups = %#"PRIx32" "
> - "actions=0x%08"PRIx32"\n",
> - features.max_groups[3], features.actions[3]);
> + for (i = 0; i < 4; i++) {
Using literal 4 here and the OVS_NOT_REACHED in group_type_to_string seems a
bit fragile, we did not abort before if the features.types has unknown bits
set. I see that the number 4 is hardwired also in the abstract group features
structure. Maybe define a macro/enum for “4”?
> + if (features.types & (1u << i)) {
> + ds_put_format(string, " %s group:\n",
> group_type_to_string(i));
> + ds_put_format(string, " max_groups=%#"PRIx32"\n",
> + features.max_groups[i]);
> + ds_put_format(string, " actions: ");
> + ofpact_bitmap_format(features.ofpacts[i], string);
> + ds_put_char(string, '\n');
> + }
> }
> }
(snip)
> @@ -1099,9 +1085,7 @@ struct ofputil_group_features {
> uint32_t types; /* Bitmap of OFPGT_* values supported. */
> uint32_t capabilities; /* Bitmap of OFPGFC12_* capability supported.
> */
> uint32_t max_groups[4]; /* Maximum number of groups for each type. */
> -
> - /* Bitmaps of OFPAT_* that are supported. OF1.2+ actions only. */
> - uint32_t actions[4];
> + uint64_t ofpacts[4]; /* Bitmaps of supported OFPACT_* */
> };
>
> /* Group desc reply, independent of protocol. */
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
