On Thu, Aug 07, 2014 at 04:03:21PM -0700, Jarno Rajahalme wrote:
> 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.
I guess that "map" is better than "xlate" from that point of view. I
did a search-and-replace. Here is what I ended up with on the
comments:
/* Two-way translation between OVS's internal "OFPACT_*" representation of
* actions and the "OFPAT_*" representation used in some OpenFlow version.
* (OFPAT_* numbering varies from one OpenFlow version to another, so a given
* instance is specific to one OpenFlow version.) */
struct ofpact_map {
enum ofpact_type ofpact; /* Internal name for action type. */
int ofpat; /* OFPAT_* number from OpenFlow spec. */
};
> > +};
> > +
> > +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.
Thanks, added:
* 4. <NAME>, a quoted string that gives the name of the action, for use in
* parsing actions from text.
> > - 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??
It still does not abort if features.types has unknown bits: the bits
that are understood (bits 0, 1, 2, 3) are the same as the bits that
the loop checks for.
A macro is a good idea. I folded that in:
diff --git a/include/openflow/openflow-1.2.h b/include/openflow/openflow-1.2.h
index 1069f4e..c7ba6fd 100644
--- a/include/openflow/openflow-1.2.h
+++ b/include/openflow/openflow-1.2.h
@@ -306,12 +306,20 @@ struct ofp12_table_stats {
};
OFP_ASSERT(sizeof(struct ofp12_table_stats) == 128);
+/* Number of types of groups supported by ofp12_group_features_stats. */
+#define OFPGT12_N_TYPES 4
+
/* Body of reply to OFPST12_GROUP_FEATURES request. Group features. */
struct ofp12_group_features_stats {
ovs_be32 types; /* Bitmap of OFPGT11_* values supported. */
ovs_be32 capabilities; /* Bitmap of OFPGFC12_* capability supported. */
- ovs_be32 max_groups[4]; /* Maximum number of groups for each type. */
- ovs_be32 actions[4]; /* Bitmaps of OFPAT_* that are supported. */
+
+ /* Each element in the following arrays corresponds to the group type with
+ * the same number, e.g. max_groups[0] is the maximum number of OFPGT11_ALL
+ * groups, actions[2] is the actions supported by OFPGT11_INDIRECT
+ * groups. */
+ ovs_be32 max_groups[OFPGT12_N_TYPES]; /* Max number of groups. */
+ ovs_be32 actions[OFPGT12_N_TYPES]; /* Bitmaps of supported OFPAT_*. */
};
OFP_ASSERT(sizeof(struct ofp12_group_features_stats) == 40);
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2443,7 +2443,7 @@ ofp_print_group_features(struct ds *string, const struct
ofp_header *oh)
ds_put_format(string, " Capabilities: 0x%"PRIx32"\n",
features.capabilities);
- for (i = 0; i < 4; i++) {
+ for (i = 0; i < OFPGT12_N_TYPES; i++) {
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",
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 0ef4070..9a8a868 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -6799,7 +6799,7 @@ ofputil_encode_group_features_reply(
ogf = ofpbuf_put_zeros(reply, sizeof *ogf);
ogf->types = htonl(features->types);
ogf->capabilities = htonl(features->capabilities);
- for (i = 0; i < 4; i++) {
+ for (i = 0; i < OFPGT12_N_TYPES; i++) {
ogf->max_groups[i] = htonl(features->max_groups[i]);
ogf->actions[i] = ofpact_bitmap_to_openflow(features->ofpacts[i],
request->version);
@@ -6818,7 +6818,7 @@ ofputil_decode_group_features_reply(const struct
ofp_header *oh,
features->types = ntohl(ogf->types);
features->capabilities = ntohl(ogf->capabilities);
- for (i = 0; i < 4; i++) {
+ for (i = 0; i < OFPGT12_N_TYPES; i++) {
features->max_groups[i] = ntohl(ogf->max_groups[i]);
features->ofpacts[i] = ofpact_bitmap_from_openflow(
ogf->actions[i], oh->version);
I pushed this to master.
Thank you for the review!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
