[ovs-dev] [PATCH 2/5] ofproto: Add support for OF1.3 port description multipart message.
Justin Pettit
jpettit at nicira.com
Tue May 8 06:19:42 UTC 2012
On May 7, 2012, at 5:11 PM, Ben Pfaff wrote:
> I noticed one more thing. ofp_print_switch_features() sorts the ports
> that it prints by port number. It might be nice for
> ofp_print_ofpst_port_desc_reply() to do the same. Sure, it wouldn't
> be globally sorted, only within a single reply message, but that still
> might help to make the output more readable. And it would definitely
> make single-message replies more readable.
I thought about that, but since it wasn't going to be globally sorted, I decided to skip it. However, having that many ports is unusual, so we may as well make the common case pretty. There's an incremental at the end of this message. If we want to be consistent, we should probably do something similar for "dump-ports", since those aren't sorted.
By the way, I think we had a memory leak in the error handling of ofp_print_switch_features() that should be fixed in this patch. I'll send a separate patch for 1.6 and earlier.
--Justin
---------
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 9539568..2805b84 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -642,6 +642,37 @@ ofp_print_phy_port(struct ds *string, const struct ofputil_
port->max_speed / UINT32_C(1000));
}
+/* Given a buffer 'b' that contains an array of OpenFlow ports of type
+ * 'ofp_version', writes a detailed description of each port into
+ * 'string'. */
+static void
+ofp_print_phy_ports(struct ds *string, uint8_t ofp_version,
+ struct ofpbuf *b)
+{
+ size_t n_ports;
+ struct ofputil_phy_port *ports;
+ enum ofperr error;
+ size_t i;
+
+ n_ports = ofputil_count_phy_ports(ofp_version, b);
+
+ ports = xmalloc(n_ports * sizeof *ports);
+ for (i = 0; i < n_ports; i++) {
+ error = ofputil_pull_phy_port(ofp_version, b, &ports[i]);
+ if (error) {
+ ofp_print_error(string, error);
+ goto exit;
+ }
+ }
+ qsort(ports, n_ports, sizeof *ports, compare_ports);
+ for (i = 0; i < n_ports; i++) {
+ ofp_print_phy_port(string, &ports[i]);
+ }
+
+exit:
+ free(ports);
+}
+
static const char *
ofputil_capabilities_to_name(uint32_t bit)
{
@@ -704,11 +735,8 @@ ofp_print_switch_features(struct ds *string,
const struct ofp_switch_features *osf)
{
struct ofputil_switch_features features;
- struct ofputil_phy_port *ports;
enum ofperr error;
struct ofpbuf b;
- size_t n_ports;
- size_t i;
error = ofputil_decode_switch_features(osf, &features, &b);
if (error) {
@@ -730,21 +758,7 @@ ofp_print_switch_features(struct ds *string,
ofputil_action_bitmap_to_name);
ds_put_char(string, '\n');
- n_ports = ofputil_count_phy_ports(osf->header.version, &b);
-
- ports = xmalloc(n_ports * sizeof *ports);
- for (i = 0; i < n_ports; i++) {
- error = ofputil_pull_phy_port(osf->header.version, &b, &ports[i]);
- if (error) {
- ofp_print_error(string, error);
- return;
- }
- }
- qsort(ports, n_ports, sizeof *ports, compare_ports);
- for (i = 0; i < n_ports; i++) {
- ofp_print_phy_port(string, &ports[i]);
- }
- free(ports);
+ ofp_print_phy_ports(string, osf->header.version, &b);
}
static void
@@ -1394,31 +1408,13 @@ ofp_print_ofpst_queue_reply(struct ds *string, const str
static void
ofp_print_ofpst_port_desc_reply(struct ds *string,
- const struct ofp_header *oh, int verbosity)
+ const struct ofp_header *oh)
{
- size_t n_ports;
- enum ofperr error;
- struct ofputil_phy_port pp;
struct ofpbuf b;
- int i;
ofpbuf_use_const(&b, oh, ntohs(oh->length));
ofpbuf_pull(&b, sizeof(struct ofp_stats_msg));
-
- n_ports = ofputil_count_phy_ports(oh->version, &b);
- ds_put_format(string, " %zu ports\n", n_ports);
- if (verbosity < 1) {
- return;
- }
-
- for (i = 0; i < n_ports; i++) {
- error = ofputil_pull_phy_port(oh->version, &b, &pp);
- if (error) {
- ofp_print_error(string, error);
- return;
- }
- ofp_print_phy_port(string, &pp);
- }
+ ofp_print_phy_ports(string, oh->version, &b);
}
static void
@@ -1744,7 +1740,7 @@ ofp_to_string__(const struct ofp_header *oh,
case OFPUTIL_OFPST_PORT_DESC_REPLY:
ofp_print_stats_reply(string, oh);
- ofp_print_ofpst_port_desc_reply(string, oh, verbosity);
+ ofp_print_ofpst_port_desc_reply(string, oh);
break;
case OFPUTIL_NXT_ROLE_REQUEST:
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 1714307..897f18b 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -703,8 +703,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
00 00 00 00 00 00 00 01 00 00 00 01 00 00 02 08 \
00 00 02 8f 00 00 02 8f 00 00 00 00 \
"], [0], [dnl
-OFPST_PORT_DESC reply (xid=0x0): 1 ports
- 3(eth0): addr:50:54:00:00:00:01
+OFPST_PORT_DESC reply (xid=0x0): 3(eth0): addr:50:54:00:00:00:01
config: PORT_DOWN
state: LINK_DOWN
current: 100MB-FD AUTO_NEG
@@ -730,8 +729,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
00 00 00 00 00 00 20 08 00 00 28 0f 00 00 28 0f \
00 00 00 00 00 01 86 a0 00 01 86 a0 \
"], [0], [dnl
-OFPST_PORT_DESC reply (OF1.1) (xid=0x0): 1 ports
- 3(eth0): addr:50:54:00:00:00:01
+OFPST_PORT_DESC reply (OF1.1) (xid=0x0): 3(eth0): addr:50:54:00:00:00:01
config: 0
state: 0
current: 100MB-FD AUTO_NEG
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 7db7f18..d0c9544 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -42,8 +42,7 @@ AT_SETUP([ofproto - port-desc stats])
OVS_VSWITCHD_START
AT_CHECK([ovs-ofctl -vwarn dump-ports-desc br0], [0], [stdout])
AT_CHECK([STRIP_XIDS stdout], [0], [dnl
-OFPST_PORT_DESC reply: 1 ports
- LOCAL(br0): addr:aa:55:aa:55:00:00
+OFPST_PORT_DESC reply: LOCAL(br0): addr:aa:55:aa:55:00:00
config: PORT_DOWN
state: LINK_DOWN
speed: 100 Mbps now, 100 Mbps max
More information about the dev
mailing list