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]
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
