On Wed, Oct 30, 2013 at 2:13 PM, Ben Pfaff <b...@nicira.com> wrote:
> This new function will have an additional caller in an upcoming commit.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
This fails to compile for me. The arguments for ofproto_trace() has mis-matches.
Otherwise looks good.
> ---
> ofproto/ofproto-dpif.c | 109
> +++++++++++++++++++++++++++++-------------------
> tests/ofproto-dpif.at | 8 ++--
> 2 files changed, 70 insertions(+), 47 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 1fd1ecd..7e1d057 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5126,37 +5126,39 @@ trace_report(struct xlate_in *xin, const char *s, int
> recurse)
> ds_put_char(result, '\n');
> }
>
> -static void
> -ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char
> *argv[],
> - void *aux OVS_UNUSED)
> +/* Parses the 'argc' elements of 'argv', ignoring argv[0]. The following
> + * forms are supported:
> + *
> + * - [dpname] odp_flow [-generate | packet]
> + * - bridge br_flow [-generate | packet]
> + *
> + * On success, initializes '*ofprotop' and 'flow' and returns NULL. On
> failure
> + * returns a nonnull error message. */
> +static const char *
> +parse_flow_and_packet(int argc, const char *argv[],
> + struct ofproto_dpif **ofprotop, struct flow *flow,
> + struct ofpbuf **packetp)
> {
> const struct dpif_backer *backer = NULL;
> - struct ofproto_dpif *ofproto;
> - struct ofpbuf odp_key, odp_mask;
> + const char *error = NULL;
> + struct simap port_names = SIMAP_INITIALIZER(&port_names);
> struct ofpbuf *packet;
> - struct ds result;
> - struct flow flow;
> - struct simap port_names;
> - char *s;
> + struct ofpbuf odp_key;
> + struct ofpbuf odp_mask;
>
> - packet = NULL;
> - backer = NULL;
> - ds_init(&result);
> ofpbuf_init(&odp_key, 0);
> ofpbuf_init(&odp_mask, 0);
> - simap_init(&port_names);
>
> /* Handle "-generate" or a hex string as the last argument. */
> if (!strcmp(argv[argc - 1], "-generate")) {
> packet = ofpbuf_new(0);
> argc--;
> } else {
> - const char *error = eth_from_hex(argv[argc - 1], &packet);
> + error = eth_from_hex(argv[argc - 1], &packet);
> if (!error) {
> argc--;
> } else if (argc == 4) {
> /* The 3-argument form must end in "-generate' or a hex string.
> */
> - unixctl_command_reply_error(conn, error);
> goto exit;
> }
> }
> @@ -5173,12 +5175,15 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int
> argc, const char *argv[],
> dp_type = argv[1];
> }
> backer = shash_find_data(&all_dpif_backers, dp_type);
> - } else {
> + } else if (argc == 2) {
> struct shash_node *node;
> if (shash_count(&all_dpif_backers) == 1) {
> node = shash_first(&all_dpif_backers);
> backer = node->data;
> }
> + } else {
> + error = "Syntax error";
> + goto exit;
> }
> if (backer && backer->dpif) {
> struct dpif_port dpif_port;
> @@ -5193,63 +5198,83 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int
> argc, const char *argv[],
> * bridge is specified. If function odp_flow_key_from_string()
> * returns 0, the flow is a odp_flow. If function
> * parse_ofp_exact_flow() returns 0, the flow is a br_flow. */
> - if (!odp_flow_from_string(argv[argc - 1], &port_names, &odp_key,
> &odp_mask)) {
> + if (!odp_flow_from_string(argv[argc - 1], &port_names,
> + &odp_key, &odp_mask)) {
> if (!backer) {
> - unixctl_command_reply_error(conn, "Cannot find the datapath");
> + error = "Cannot find the datapath";
> goto exit;
> }
>
> - if (xlate_receive(backer, NULL, odp_key.data, odp_key.size, &flow,
> - NULL, &ofproto, NULL)) {
> - unixctl_command_reply_error(conn, "Invalid datapath flow");
> + if (xlate_receive(backer, NULL, odp_key.data, odp_key.size, flow,
> + NULL, ofprotop, NULL)) {
> + error = "Invalid datapath flow";
> goto exit;
> }
> - ds_put_format(&result, "Bridge: %s\n", ofproto->up.name);
> - } else if (!parse_ofp_exact_flow(&flow, NULL, argv[argc - 1], NULL)) {
> + } else if (!parse_ofp_exact_flow(flow, NULL, argv[argc - 1], NULL)) {
> if (argc != 3) {
> - unixctl_command_reply_error(conn, "Must specify bridge name");
> + error = "Must specify bridge name";
> goto exit;
> }
>
> - ofproto = ofproto_dpif_lookup(argv[1]);
> - if (!ofproto) {
> - unixctl_command_reply_error(conn, "Unknown bridge name");
> + *ofprotop = ofproto_dpif_lookup(argv[1]);
> + if (!*ofprotop) {
> + error = "Unknown bridge name";
> goto exit;
> }
> } else {
> - unixctl_command_reply_error(conn, "Bad flow syntax");
> + error = "Bad flow syntax";
> goto exit;
> }
>
> /* Generate a packet, if requested. */
> if (packet) {
> if (!packet->size) {
> - flow_compose(packet, &flow);
> + flow_compose(packet, flow);
> } else {
> - union flow_in_port in_port_;
> -
> - in_port_ = flow.in_port;
> - ds_put_cstr(&result, "Packet: ");
> - s = ofp_packet_to_string(packet->data, packet->size);
> - ds_put_cstr(&result, s);
> - free(s);
> + union flow_in_port in_port = flow->in_port;
>
> /* Use the metadata from the flow and the packet argument
> * to reconstruct the flow. */
> - flow_extract(packet, flow.skb_priority, flow.pkt_mark, NULL,
> - &in_port_, &flow);
> + flow_extract(packet, flow->skb_priority, flow->pkt_mark, NULL,
> + &in_port, flow);
> }
> }
>
> - ofproto_trace(ofproto, &flow, packet, &result);
> - unixctl_command_reply(conn, ds_cstr(&result));
> + error = NULL;
>
> exit:
> - ds_destroy(&result);
> - ofpbuf_delete(packet);
> + if (error) {
> + ofpbuf_delete(packet);
> + packet = NULL;
> + }
> + *packetp = packet;
> ofpbuf_uninit(&odp_key);
> ofpbuf_uninit(&odp_mask);
> simap_destroy(&port_names);
> + return error;
> +}
> +
> +static void
> +ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char
> *argv[],
> + void *aux OVS_UNUSED)
> +{
> + struct ofproto_dpif *ofproto;
> + struct ofpbuf *packet;
> + const char *error;
> + struct flow flow;
> +
> + error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
> + if (!error) {
> + struct ds result;
> +
> + ds_init(&result);
> + ofproto_trace(ofproto, &flow, packet, NULL, 0, &result);
> + unixctl_command_reply(conn, ds_cstr(&result));
> + ds_destroy(&result);
> + ofpbuf_delete(packet);
> + } else {
> + unixctl_command_reply_error(conn, error);
> + }
> }
>
> static void
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 9e0ea4b..4ae1054 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -1357,9 +1357,8 @@ AT_CHECK([ovs-appctl ofproto/trace \
> AT_CHECK([tail -1 stdout], [0], [dnl
> Datapath actions: 2
> ])
> -AT_CHECK([head -n 3 stdout], [0], [dnl
> +AT_CHECK([head -n 2 stdout], [0], [dnl
> Bridge: br0
> -Packet:
> arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:01,dl_dst=50:54:00:00:00:02,arp_spa=0.0.0.0,arp_tpa=0.0.0.0,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> Flow:
> pkt_mark=0x2,skb_priority=0x1,arp,metadata=0,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:01,dl_dst=50:54:00:00:00:02,arp_spa=0.0.0.0,arp_tpa=0.0.0.0,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> ])
>
> @@ -1369,9 +1368,8 @@ AT_CHECK([ovs-appctl ofproto/trace ovs-dummy \
> AT_CHECK([tail -1 stdout], [0], [dnl
> Datapath actions: 2
> ])
> -AT_CHECK([head -n 3 stdout], [0], [dnl
> +AT_CHECK([head -n 2 stdout], [0], [dnl
> Bridge: br0
> -Packet:
> arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:01,dl_dst=50:54:00:00:00:02,arp_spa=0.0.0.0,arp_tpa=0.0.0.0,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> Flow:
> pkt_mark=0x2,skb_priority=0x1,arp,metadata=0,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:01,dl_dst=50:54:00:00:00:02,arp_spa=0.0.0.0,arp_tpa=0.0.0.0,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> ])
>
> @@ -1382,7 +1380,7 @@ AT_CHECK([tail -1 stdout], [0], [dnl
> Datapath actions: 1
> ])
> AT_CHECK([head -n 2 stdout], [0], [dnl
> -Packet:
> arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:02,dl_dst=50:54:00:00:00:01,arp_spa=0.0.0.0,arp_tpa=0.0.0.0,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> +Bridge: br0
> Flow:
> pkt_mark=0x1,skb_priority=0x2,arp,metadata=0,in_port=2,vlan_tci=0x0000,dl_src=50:54:00:00:00:02,dl_dst=50:54:00:00:00:01,arp_spa=0.0.0.0,arp_tpa=0.0.0.0,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> ])
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
