Maybe the title should be “ofp-print: Print all async config messages the same way.” instead?
Acked-by: Jarno Rajahalme <ja...@ovn.org> > On Jan 18, 2016, at 11:27 PM, Ben Pfaff <b...@ovn.org> wrote: > > We have a single function to decode all of these messages, so there's no > reason to do it two different ways for printing. > > Signed-off-by: Ben Pfaff <b...@ovn.org> > --- > lib/ofp-print.c | 117 +++++++++++--------------------------------------- > tests/ofp-print.at | 26 +++++++++-- > tests/ofproto-dpif.at | 6 +++ > tests/ofproto.at | 6 +++ > 4 files changed, 60 insertions(+), 95 deletions(-) > > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > index f36335b..bedfc94 100644 > --- a/lib/ofp-print.c > +++ b/lib/ofp-print.c > @@ -2063,108 +2063,42 @@ ofp_async_config_reason_to_string(uint32_t reason, > > #define OFP_ASYNC_CONFIG_REASON_BUFSIZE (INT_STRLEN(int) + 1) > static void > -ofp_print_nxt_set_async_config(struct ds *string, > - const struct ofp_header *oh) > +ofp_print_set_async_config(struct ds *string, const struct ofp_header *oh, > + enum ofptype type) > { > - int i, j; > - enum ofpraw raw; > - > - ofpraw_decode(&raw, oh); > - > - if (raw == OFPRAW_OFPT13_SET_ASYNC || > - raw == OFPRAW_NXT_SET_ASYNC_CONFIG || > - raw == OFPRAW_OFPT13_GET_ASYNC_REPLY) { > - const struct nx_async_config *nac = ofpmsg_body(oh); > - > - for (i = 0; i < 2; i++) { > + struct ofputil_async_cfg basis = OFPUTIL_ASYNC_CFG_INIT; > + struct ofputil_async_cfg ac; > > - ds_put_format(string, "\n %s:\n", i == 0 ? "master" : "slave"); > - > - ds_put_cstr(string, " PACKET_IN:"); > - for (j = 0; j < 32; j++) { > - if (nac->packet_in_mask[i] & htonl(1u << j)) { > - char reasonbuf[OFPUTIL_PACKET_IN_REASON_BUFSIZE]; > - const char *reason; > - > - reason = ofputil_packet_in_reason_to_string(j, reasonbuf, > - sizeof > reasonbuf); > - ds_put_format(string, " %s", reason); > - } > - } > - if (!nac->packet_in_mask[i]) { > - ds_put_cstr(string, " (off)"); > - } > - ds_put_char(string, '\n'); > - > - ds_put_cstr(string, " PORT_STATUS:"); > - for (j = 0; j < 32; j++) { > - if (nac->port_status_mask[i] & htonl(1u << j)) { > - char reasonbuf[OFP_PORT_REASON_BUFSIZE]; > - const char *reason; > + bool is_reply = type == OFPTYPE_GET_ASYNC_REPLY; > + enum ofperr error = ofputil_decode_set_async_config(oh, is_reply, > + &basis, &ac); > + if (error) { > + ofp_print_error(string, error); > + return; > + } > > - reason = ofp_port_reason_to_string(j, reasonbuf, > - sizeof reasonbuf); > - ds_put_format(string, " %s", reason); > - } > - } > - if (!nac->port_status_mask[i]) { > - ds_put_cstr(string, " (off)"); > - } > - ds_put_char(string, '\n'); > + for (int i = 0; i < 2; i++) { > + ds_put_format(string, "\n %s:\n", i == 0 ? "master" : "slave"); > + for (uint32_t type = 0; type < OAM_N_TYPES; type++) { > + ds_put_format(string, "%16s:", > + ofputil_async_msg_type_to_string(type)); > > - ds_put_cstr(string, " FLOW_REMOVED:"); > - for (j = 0; j < 32; j++) { > - if (nac->flow_removed_mask[i] & htonl(1u << j)) { > - char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE]; > + uint32_t role = i == 0 ? ac.master[type] : ac.slave[type]; > + for (int j = 0; j < 32; j++) { > + if (role & (1u << j)) { > + char reasonbuf[OFP_ASYNC_CONFIG_REASON_BUFSIZE]; > const char *reason; > > - reason = ofp_flow_removed_reason_to_string(j, reasonbuf, > - sizeof reasonbuf); > + reason = ofp_async_config_reason_to_string( > + j, type, reasonbuf, sizeof reasonbuf); > ds_put_format(string, " %s", reason); > } > } > - if (!nac->flow_removed_mask[i]) { > + if (!role) { > ds_put_cstr(string, " (off)"); > } > ds_put_char(string, '\n'); > } > - } else if (raw == OFPRAW_OFPT14_SET_ASYNC || > - raw == OFPRAW_OFPT14_GET_ASYNC_REPLY) { > - struct ofputil_async_cfg basis = OFPUTIL_ASYNC_CFG_INIT; > - struct ofputil_async_cfg ac; > - > - bool is_reply = raw == OFPRAW_OFPT14_GET_ASYNC_REPLY; > - enum ofperr error = ofputil_decode_set_async_config(oh, is_reply, > - &basis, &ac); > - if (error) { > - ofp_print_error(string, error); > - return; > - } > - > - for (i = 0; i < 2; i++) { > - ds_put_format(string, "\n %s:\n", i == 0 ? "master" : "slave"); > - for (uint32_t type = 0; type < OAM_N_TYPES; type++) { > - ds_put_format(string, "%16s:", > - ofputil_async_msg_type_to_string(type)); > - > - uint32_t role = i == 0 ? ac.master[type] : ac.slave[type]; > - for (j = 0; j < 32; j++) { > - if (role & (1u << j)) { > - char reasonbuf[OFP_ASYNC_CONFIG_REASON_BUFSIZE]; > - const char *reason; > - > - reason = ofp_async_config_reason_to_string(j, type, > - reasonbuf, > - sizeof reasonbuf); > - ds_put_format(string, " %s", reason); > - } > - } > - if (!role) { > - ds_put_cstr(string, " (off)"); > - } > - ds_put_char(string, '\n'); > - } > - } > } > } > > @@ -3137,8 +3071,9 @@ ofp_to_string__(const struct ofp_header *oh, enum > ofpraw raw, > const void *msg = oh; > > ofp_header_to_string__(oh, raw, string); > - switch (ofptype_from_ofpraw(raw)) { > > + enum ofptype type = ofptype_from_ofpraw(raw); > + switch (type) { > case OFPTYPE_GROUP_STATS_REQUEST: > ofp_print_stats(string, oh); > ofp_print_ofpst_group_request(string, oh); > @@ -3373,7 +3308,7 @@ ofp_to_string__(const struct ofp_header *oh, enum > ofpraw raw, > > case OFPTYPE_GET_ASYNC_REPLY: > case OFPTYPE_SET_ASYNC_CONFIG: > - ofp_print_nxt_set_async_config(string, oh); > + ofp_print_set_async_config(string, oh, type); > break; > case OFPTYPE_GET_ASYNC_REQUEST: > break; > diff --git a/tests/ofp-print.at b/tests/ofp-print.at > index 6d508d3..c791cb2 100644 > --- a/tests/ofp-print.at > +++ b/tests/ofp-print.at > @@ -2637,20 +2637,30 @@ AT_CLEANUP > > AT_SETUP([OFPT_SET_ASYNC - OF1.3]) > AT_KEYWORDS([ofp-print]) > +dnl This message has bit 12 set for the PACKET_IN messages (master and > slave). > +dnl Those aren't supported bits so they get silently ignored on decoding. > +dnl That seems reasonable because OF1.3 doesn't define any error codes for > +dnl OFPT_SET_ASYNC. > AT_CHECK([ovs-ofctl ofp-print "\ > 04 1c 00 20 00 00 00 00 00 00 10 05 00 00 10 07 \ > 00 00 00 03 00 00 00 07 00 00 00 00 00 00 00 03 \ > "], [0], [dnl > OFPT_SET_ASYNC (OF1.3) (xid=0x0): > master: > - PACKET_IN: no_match invalid_ttl 12 > + PACKET_IN: no_match invalid_ttl > PORT_STATUS: add delete > FLOW_REMOVED: (off) > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > > slave: > - PACKET_IN: no_match action invalid_ttl 12 > + PACKET_IN: no_match action invalid_ttl > PORT_STATUS: add delete modify > FLOW_REMOVED: idle hard > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > ]) > AT_CLEANUP > > @@ -2838,6 +2848,8 @@ AT_CLEANUP > > AT_SETUP([NXT_SET_ASYNC_CONFIG]) > AT_KEYWORDS([ofp-print]) > +dnl This message has bit 12 set for the PACKET_IN messages (master and > slave). > +dnl Those aren't supported bits so they get silently ignored on decoding. > AT_CHECK([ovs-ofctl ofp-print "\ > 01 04 00 28 00 00 00 00 00 00 23 20 00 00 00 13 \ > 00 00 10 05 00 00 10 07 00 00 00 03 00 00 00 07 \ > @@ -2845,14 +2857,20 @@ AT_CHECK([ovs-ofctl ofp-print "\ > "], [0], [dnl > NXT_SET_ASYNC_CONFIG (xid=0x0): > master: > - PACKET_IN: no_match invalid_ttl 12 > + PACKET_IN: no_match invalid_ttl > PORT_STATUS: add delete > FLOW_REMOVED: (off) > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > > slave: > - PACKET_IN: no_match action invalid_ttl 12 > + PACKET_IN: no_match action invalid_ttl > PORT_STATUS: add delete modify > FLOW_REMOVED: idle hard > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > ]) > AT_CLEANUP > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 4c2a995..bc1af8f 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -2810,11 +2810,17 @@ send: OFPT_SET_ASYNC (OF1.3) (xid=0x2): > PACKET_IN: (off) > PORT_STATUS: (off) > FLOW_REMOVED: (off) > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > > slave: > PACKET_IN: no_match > PORT_STATUS: (off) > FLOW_REMOVED: (off) > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > dnl > OFPT_PACKET_IN (OF1.3) (xid=0x0): cookie=0x0 total_len=60 in_port=1 (via > no_match) data_len=60 (unbuffered) > tcp,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn > tcp_csum:0 > diff --git a/tests/ofproto.at b/tests/ofproto.at > index ab4d254..61a6be5 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -3845,11 +3845,17 @@ OFPT_GET_ASYNC_REPLY (OF1.3): > PACKET_IN: no_match action > PORT_STATUS: add delete modify > FLOW_REMOVED: idle hard delete > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > > slave: > PACKET_IN: (off) > PORT_STATUS: add delete modify > FLOW_REMOVED: (off) > + ROLE_STATUS: (off) > + TABLE_STATUS: (off) > + REQUESTFORWARD: (off) > OFPT_BARRIER_REPLY (OF1.3): > ]) > > -- > 2.1.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev