[ovs-dev] [of1.1 v5 1/5] ofp-msgs: New approach to encoding and decoding OpenFlow headers.

[Simon Horman]

On Thu, Jul 05, 2012 at 11:12:29PM -0700, Ben Pfaff wrote:
> OpenFlow headers are not as uniform as they could be, with size, alignment,
> and numbering changes from one version to another and across varieties
> (e.g. ordinary messages vs. "stats" messages).  Until now the Open vSwitch
> internal APIs haven't done a good job of abstracting those differences in
> header formats.  This commit changes that; from this commit forward very
> little code actually needs to understand the header format or numbering.
> Instead, it can just encode or decode, or pull or put, the header using
> a more abstract API using the ofpraw_, ofptype_, and other APIs in the
> new ofp-msgs module.
> 
> Signed-off-by: Ben Pfaff <blp at nicira.com>

[snip]

> diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
> new file mode 100644
> index 0000000..439a7d6
> --- /dev/null
> +++ b/lib/ofp-msgs.h
> @@ -0,0 +1,413 @@

[snip]

> +/* Raw identifiers for OpenFlow messages.
> + *
> + * Some OpenFlow messages with similar meanings have multiple variants across
> + * OpenFlow versions or vendor extensions.  Each variant has a different
> + * OFPRAW_* enumeration constant.  More specifically, if two messages have
> + * different types, different numbers, or different arguments, then they must
> + * have different OFPRAW_* values.
> + *
> + * The comments here must follow a stylized form because the "extract-ofp-msgs"
> + * program parses them at build time to generate data tables.  The syntax of
> + * each comment is:
> + *
> + *    type versions (number): arguments.
> + *
> + * where the syntax of each part is:
> + *
> + *    - type: One of OFPT (standard OpenFlow message), OFPST (standard OpenFlow
> + *      statistics message), NXT (Nicira extension message), or NXST (Nicira
> + *      extension statistics message).
> + *
> + *      As new vendors implement extensions it will make sense to expand the
> + *      dictionary of possible types.
> + *
> + *    - versions: The OpenFlow version or versions in which this message is
> + *      supported, e.g. "1.0" or "1.1" or "1.0+".
> + *
> + *    - number:
> + *         For OFPT, the 'type' in struct ofp_header.
> + *         For OFPST, the 'type' in struct ofp_stats_msg or ofp11_stats_msg.
> + *         For NXT, the 'subtype' in struct nicira_header.
> + *         For NXST, the 'subtype' in struct nicira10_stats_msg or
> + *           nicira11_stats_msg.
> + *
> + *    - arguments: The types of data that follow the OpenFlow headers (the
> + *      message "body").  This can be "void" if the message has no body.
> + *      Otherwise, it should be a comma-separated sequence of C types.  The
> + *      last type in the sequence can end with [] if the body ends in a
> + *      variable-length sequence.
> + *
> + *      The arguments are used to validate the lengths of messages when a
> + *      header is parsed.  Any message whose length isn't valid as a length of
> + *      the specified types will be rejected with OFPERR_OFPBRC_BAD_LEN.
> + *
> + *      A few OpenFlow messages, such as OFPT_PACKET_IN, intentionally end with
> + *      only part of a structure, up to some specified member.  The syntax "up
> + *      to <member>" indicates this, e.g. "struct ofp11_packet_in up to data".
> + */
> +enum ofpraw {
> +/* Standard messages. */
> +
> +    /* OFPT 1.0+ (0): uint8_t[]. */
> +    OFPRAW_OFPT_HELLO,
> +
> +    /* OFPT 1.0+ (1): struct ofp_error_msg, uint8_t[]. */
> +    OFPRAW_OFPT_ERROR,
> +
> +    /* OFPT 1.0+ (2): uint8_t[]. */
> +    OFPRAW_OFPT_ECHO_REQUEST,
> +
> +    /* OFPT 1.0+ (3): uint8_t[]. */
> +    OFPRAW_OFPT_ECHO_REPLY,
> +
> +    /* OFPT 1.0+ (5): void. */
> +    OFPRAW_OFPT_FEATURES_REQUEST,
> +
> +    /* OFPT 1.0 (6): struct ofp_switch_features, struct ofp10_phy_port[]. */
> +    OFPRAW_OFPT10_FEATURES_REPLY,
> +    /* OFPT 1.1+ (6): struct ofp_switch_features, struct ofp11_port[]. */
> +    OFPRAW_OFPT11_FEATURES_REPLY,
> +
> +    /* OFPT 1.0+ (7): void. */
> +    OFPRAW_OFPT_GET_CONFIG_REQUEST,
> +
> +    /* OFPT 1.0+ (8): struct ofp_switch_config. */
> +    OFPRAW_OFPT_GET_CONFIG_REPLY,
> +
> +    /* OFPT 1.0-1.1 (9): struct ofp_switch_config. */
> +    OFPRAW_OFPT_SET_CONFIG,
> +
> +    /* OFPT 1.0 (10): struct ofp_packet_in up to data, uint8_t[]. */
> +    OFPRAW_OFPT10_PACKET_IN,
> +    /* OFPT 1.1 (11): struct ofp11_packet_in up to data, uint8_t[]. */
> +    OFPRAW_OFPT11_PACKET_IN,

I believe that OFPRAW_OFPT11_PACKET_IN's number should be 10.

> +    /* NXT 1.0+ (17): struct nx_packet_in, uint8_t[]. */
> +    OFPRAW_NXT_PACKET_IN,
> +
> +    /* OFPT 1.0 (11): struct ofp_flow_removed. */
> +    OFPRAW_OFPT10_FLOW_REMOVED,
> +    /* NXT 1.0+ (14): struct nx_flow_removed, uint8_t[8][]. */
> +    OFPRAW_NXT_FLOW_REMOVED,

I'm unsure how important the arguments are.

For example, in the case of Flow Removed, OF1.1 has a 40byte
struct ofp11_flow_removed followed by an 88 byte struct ofp11_match.
OF1.2 has a 40 byte struct ofp13_flow_removed, followed by a 4 byte
ofp12_match, followed by uing8_t[], followed by a pad to ensure 8 byte
alignment.

In this case should both be covered by something like this?

    OFPRAW_OFPT11_FLOW_REMOVED,
    /* OFPT 1.2 (11): struct ofp11_flow_removed, uint8_t[8][]. */ 

Or are two separate entries needed?

    /* OFPT 1.1 (11): struct ofp11_flow_removed, struct ofp11_match[]. */
    OFPRAW_OFPT11_FLOW_REMOVED,
    /* OFPT 1.2 (11): struct ofp12_flow_removed, uint8_t[8][]. */
    OFPRAW_OFPT12_FLOW_REMOVED,

In the case of the latter I see.

./lib/ofp-msgs.h:138: Duplicate message definition for (2, 11, 0, 0, 0).
./lib/ofp-msgs.h:135: Here is the location of the previous definition.

Which seems to be a bit limiting, though I'm unsure if there is a real
use case for separately defining the OF1.1 and OF1.2 types.

[snip]