[ovs-dev] [of1.1 draft v2 1/7] Introduce ofputil_protocol, to abstract the protocol in use on a connection.

[Simon Horman]

On Fri, Feb 03, 2012 at 12:43:49PM -0800, Ben Pfaff wrote:
> Open vSwitch already handles a few different protocol variations, but it
> does so in a nonuniform manner:
> 
>   - OpenFlow 1.0 and NXM flow formats are distinguished using the NXFF_*
>     constant values from nicira-ext.h.
> 
>   - The "flow_mod_table_id" feature setting is maintained in ofproto as
>     part of an OpenFlow connection's (ofconn's) state.
> 
> There's no way to easily communicate this state among components.  It's
> not much of a problem yet, but as more protocol support is added it seems
> better to have an abstract, uniform way to represent protocol versions and
> variants.  This commit implements that by introducing a new type
> "enum ofputil_protocol".  Each ofputil_protocol value represents a variant
> of a protocol version.  Each value is a separate bit, so a single enum
> can also represent a set of protocols, which is often useful as well.
> 
> This commit needs the following improvements:
> 
>   - learning-switch.c should understand how to negotiate the protocol, so
>     that it can properly implement the ovs-controller --with-flows option
>     based on what the switch supports.
> 
>   - ovs-ofctl needs documentation updates, possibly ovs-controller too.

[snip]

> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index ef8c470..0a53695 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c

[snip]

> -int
> -ofputil_flow_format_from_string(const char *s)
> +/* Returns a string form of 'protocol', if a simple form exists (that is, if
> + * 'protocol' is either a single protocol or it is a combination of protocols
> + * that have a single abbreviation).  Otherwise, returns NULL. */
> +const char *
> +ofputil_protocol_to_string(enum ofputil_protocol protocol)
>  {
> -    return (!strcmp(s, "openflow10") ? NXFF_OPENFLOW10
> -            : !strcmp(s, "nxm") ? NXFF_NXM
> -            : -1);
> +    const struct proto_abbrev *p;
> +
> +    /* Use a "switch" statement for single-bit names so that we get a compiler
> +     * warning if we forget any. */
> +    switch (protocol) {
> +    case OFPUTIL_P_NXM:
> +        return "NXM-table_id";
> +
> +    case OFPUTIL_P_NXM_TID:
> +        return "NXM+table_id";
> +
> +    case OFPUTIL_P_OF10:
> +        return "OpenFlow10-table_id";
> +
> +    case OFPUTIL_P_OF10_TID:
> +        return "OpenFlow10+table_id";
> +    }
> +
> +    /* Check abbreviations. */
> +    for (p = proto_abbrevs; p < &proto_abbrevs[N_PROTO_ABBREVS]; p++) {
> +        if (protocol == p->protocol) {
> +            return p->name;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +/* Returns a string that represents 'protocols'.  The return value might be a
> + * comma-separated list if 'protocols' doesn't have a simple name.  The return
> + * value is "none" if 'protocols' is 0.
> + *
> + * The caller must free the returned string (with free()). */
> +char *
> +ofputil_protocols_to_string(enum ofputil_protocol protocols)
> +{
> +    struct ds s;
> +
> +    assert(!(protocols & ~OFPUTIL_P_ANY));
> +    if (protocols == 0) {
> +        return xstrdup("none");
> +    }
> +
> +    ds_init(&s);
> +    while (protocols) {
> +        const struct proto_abbrev *p;
> +        int i;
> +
> +        if (s.length) {
> +            ds_put_char(&s, ',');
> +        }
> +
> +        for (p = proto_abbrevs; p < &proto_abbrevs[N_PROTO_ABBREVS]; p++) {
> +            if ((protocols & p->protocol) == p->protocol) {
> +                ds_put_cstr(&s, p->name);
> +                protocols &= ~p->protocol;
> +                goto match;
> +            }
> +        }
> +
> +        for (i = 0; i < CHAR_BIT * sizeof(enum ofputil_protocol); i++) {
> +            enum ofputil_protocol bit = 1u << i;
> +
> +            if (protocols & bit) {
> +                ds_put_cstr(&s, ofputil_protocol_to_string(bit));

Do you need to check if the return value of ofputil_protocol_to_string()
is NULL?

> +                protocols &= ~bit;
> +                goto match;
> +            }
> +        }
> +        NOT_REACHED();
> +
> +    match: ;
> +    }
> +    return ds_steal_cstr(&s);
> +}

[snip]

> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 9b1a1b2..0cfb6ba 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c

[snip]

>  static void
> -print_packet_list(struct list *packets)
> +do_parse_flows__(struct ofputil_flow_mod *fms, size_t n_fms)
>  {
> -    struct ofpbuf *packet, *next;
> +    enum ofputil_protocol usable_protocols;
> +    enum ofputil_protocol protocol = 0;
> +    char *usable_s;
> +    size_t i;
> +
> +    usable_protocols = ofputil_flow_mod_usable_protocols(fms, n_fms);
> +    usable_s = ofputil_protocols_to_string(usable_protocols);
> +    printf("usable protocols: %s\n", usable_s);
> +    free(usable_s);
> +
> +    if (!(usable_protocols & allowed_protocols)) {
> +        ovs_fatal(0, "no usable protocol");
> +    }
> +    for (i = 0; i < sizeof(enum ofputil_protocol) * CHAR_BIT; i++) {
> +        protocol = 1 << i;
> +        if (protocol & usable_protocols & allowed_protocols) {
> +            break;
> +        }
> +    }
> +
> +    printf("chosen protocol: %s\n", ofputil_protocol_to_string(protocol));

And here

> -    LIST_FOR_EACH_SAFE (packet, next, list_node, packets) {
> -        ofp_print(stdout, packet->data, packet->size, verbosity);
> -        list_remove(&packet->list_node);
> -        ofpbuf_delete(packet);
> +    for (i = 0; i < n_fms; i++) {
> +        struct ofputil_flow_mod *fm = &fms[i];
> +        struct ofpbuf *msg;
> +
> +        msg = ofputil_encode_flow_mod(fm, protocol);
> +        ofp_print(stdout, msg->data, msg->size, verbosity);
> +        ofpbuf_delete(msg);
> +
> +        free(fm->actions);
>      }
>  }

[snip]