> On Jan 5, 2017, at 5:04 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 52c1758..017fbb1 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -23,165 +23,82 @@
> ...
> +struct oftrace_node *
> +oftrace_report(struct ovs_list *super, enum oftrace_node_type type,
> + const char *text)
> {
> - ds_put_char_multiple(result, '\t', level);
> - ds_put_format(result, "%s: ", title);
> - /* Do not report unchanged flows for resubmits. */
> - if ((level > 0 && flow_equal(&trace->xin.flow, &trace->flow))
> - || (level == 0 && flow_equal(&trace->xin.flow, trace->key))) {
> - ds_put_cstr(result, "unchanged");
> - } else {
> - flow_format(result, &trace->xin.flow);
> - trace->flow = trace->xin.flow;
> - }
> - ds_put_char(result, '\n');
> + struct oftrace_node *node = xmalloc(sizeof *node);
> + ovs_list_push_back(super, &node->node);
> + node->type = type;
> + node->name = xstrdup(text);
> + node->always_indent = false;
> + ovs_list_init(&node->subs);
> +
> + return node;
> }
I think it would be helpful to have a comment describing this function. Also
mentioning that the caller maintains ownership of 'text'.
I may be missing something, but is there anything that frees these
"oftrace_node"s either here or ofproto-dpif-xlate.c?
>
> static void
> +oftrace_node_print_details(struct ds *output,
> + const struct ovs_list *nodes, int level)
> {
> + const struct oftrace_node *sub;
> + LIST_FOR_EACH (sub, node, nodes) {
> + bool more = sub->node.next != nodes || sub->always_indent ||
> oftrace_node_type_is_terminal(sub->type);
This line is much longer than 80 characters.
> + bool title = sub->type == OFT_BRIDGE;
>
> ...
> + if (title) {
> + ds_put_char(output, '\n');
> + }
> + ds_put_char_multiple(output, ' ', (level + more) * 4);
It's clever, but, 'more' being a bool looks kind of weird to me seeing it used
for math.
> + switch (sub->type) {
> + case OFT_DETAIL:
> + ds_put_format(output, " -> %s\n", sub->name);
> + break;
> + case OFT_WARN:
> + ds_put_format(output, " >> %s\n", sub->name);
> + break;
> + case OFT_ERROR:
> + ds_put_format(output, " >>>> %s <<<<\n", sub->name);
> + break;
> + case OFT_BRIDGE:
> + ds_put_format(output, "%s\n", sub->name);
> + ds_put_char_multiple(output, ' ', (level + more) * 4);
> + ds_put_char_multiple(output, '-', strlen(sub->name));
> + ds_put_char(output, '\n');
> + break;
> + case OFT_TABLE:
> + case OFT_THAW:
> + case OFT_ACTION:
> + ds_put_format(output, "%s\n", sub->name);
> + break;
> }
The actions printing at a shallower depth than the rule looks a little odd to
me. I suppose that helps if there are a lot of actions. Regardless, I'm sure
I'll get used to it.
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 371ced4..f96468d 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> ...
> +static struct ovs_list * OVS_PRINTF_FORMAT(3, 4)
> +xlate_report(const struct xlate_ctx *ctx, enum oftrace_node_type type,
> + const char *format, ...)
> {
> - if (OVS_UNLIKELY(ctx->xin->report_hook)) {
> + struct ovs_list *subtrace = NULL;
> + if (OVS_UNLIKELY(ctx->xin->trace)) {
> va_list args;
> -
> va_start(args, format);
> - ctx->xin->report_hook(ctx->xin, ctx->indentation, format, args);
> + char *text = xvasprintf(format, args);
> + subtrace = &oftrace_report(ctx->xin->trace, type, text)->subs;
I think you need to free "text", since oftrace_report() makes a copy.
>
> +/* This is like xlate_report() for errors that are serious enough that we
> + * should log them even if we are not tracing. */
> +static void OVS_PRINTF_FORMAT(2, 3)
> +xlate_report_error(const struct xlate_ctx *ctx, const char *format, ...)
The next two functions in the source file have comments pointing out that
they're similar to xlate_report(), but xlate_report() doesn't have a comment
describing it. It might be nice to do that.
> +static struct ovs_list *
> +xlate_report_actions(const struct xlate_ctx *ctx, enum oftrace_node_type
> type,
> + const char *title,
> const struct ofpact *ofpacts, size_t ofpacts_len)
> {
> - if (OVS_UNLIKELY(ctx->xin->report_hook)) {
> + struct ovs_list *subtrace = NULL;
> + if (OVS_UNLIKELY(ctx->xin->trace)) {
> struct ds s = DS_EMPTY_INITIALIZER;
> + ds_put_format(&s, "%s: ", title);
> ofpacts_format(ofpacts, ofpacts_len, &s);
> - xlate_report(ctx, "%s: %s", title, ds_cstr(&s));
> + subtrace = &oftrace_report(ctx->xin->trace, type, ds_cstr(&s))->subs;
> + ds_destroy(&s);
> + }
> + return subtrace;
> +}
> +
> +static void
> +xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb)
> +{
> + if (OVS_UNLIKELY(ctx->xin->trace)) {
> + struct ofpbuf action_list;
> + ofpbuf_init(&action_list, 0);
> + ofpacts_execute_action_set(&action_list, &ctx->action_set);
> + if (action_list.size) {
> + struct ds s = DS_EMPTY_INITIALIZER;
> + ofpacts_format(action_list.data, action_list.size, &s);
> + xlate_report(ctx, OFT_DETAIL, "action set %s: %s",
> + verb, ds_cstr(&s));
> + ds_destroy(&s);
> + } else {
> + xlate_report(ctx, OFT_DETAIL, "action set %s empty", verb);
> + }
> + ofpbuf_uninit(&action_list);
> + }
> +}
The difference in behavior between xlate_report_actions() and
xlate_report_action_set() seem to be more than just that one has to do with
actions and the other with action sets. It might be nice to document the
differences.
> @@ -2966,6 +3106,7 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> struct ofpbuf old_stack = ctx->stack;
> union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
> struct ofpbuf old_action_set = ctx->action_set;
> + struct ovs_list *old_trace = ctx->xin->trace;
> uint64_t actset_stub[1024 / 8];
>
> ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack);
> @@ -2980,6 +3121,8 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> flow->actset_output = OFPP_UNSET;
> ctx->conntracked = false;
> clear_conntrack(flow);
> + ctx->xin->trace = xlate_report(ctx, OFT_BRIDGE,
> + "bridge(\"%s\")",
> peer->xbridge->name);
>
> /* When the patch port points to a different bridge, then the mirrors
> * for that bridge clearly apply independently to the packet, so we
> @@ -3031,6 +3174,7 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> }
> }
>
> + ctx->xin->trace = old_trace;
> if (independent_mirrors) {
> ctx->mirrors = old_mirrors;
> }
There are a few of these "old_trace" logic blocks, and I think the memory
management and linked list processing may not be handled properly.
> @@ -5603,12 +5823,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> * was done before they were frozen and should not be redone. */
> } else if (in_port && in_port->xbundle
> && xbundle_mirror_out(xbridge, in_port->xbundle)) {
> - if (ctx.xin->packet != NULL) {
> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> - VLOG_WARN_RL(&rl, "bridge %s: dropping packet received on port "
> - "%s, which is reserved exclusively for mirroring",
> - ctx.xbridge->name, in_port->xbundle->name);
> - }
> + xlate_report_error(&ctx, "dropping packet received on port "
> + "%s, which is reserved exclusively for mirroring",
> + in_port->xbundle->name);
This used to only log when "ctx.xin->packet" was not null. Was this
intentionally changed?
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index 51c05ac..fa2b130 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -83,27 +83,10 @@ struct xlate_in {
> * timeouts.) */
> uint16_t tcp_flags;
>
> - /* If nonnull, flow translation calls this function just before
> executing a
> - * resubmit or OFPP_TABLE action. In addition, disables logging of
> traces
> - * when the recursion depth is exceeded.
> - *
> - * 'rule' is the rule being submitted into. It will be null if the
> - * resubmit or OFPP_TABLE action didn't find a matching rule.
> - *
> - * 'indentation' is the resubmit recursion depth at time of invocation,
> - * suitable for indenting the output.
> - *
> - * This is normally null so the client has to set it manually after
> - * calling xlate_in_init(). */
> - void (*resubmit_hook)(struct xlate_in *, struct rule_dpif *rule,
> - int indentation);
> -
> - /* If nonnull, flow translation calls this function to report some
> - * significant decision, e.g. to explain why OFPP_NORMAL translation
> - * dropped a packet. 'indentation' is the resubmit recursion depth at
> time
> - * of invocation, suitable for indenting the output. */
> - void (*report_hook)(struct xlate_in *, int indentation,
> - const char *format, va_list args);
> + /* If nonnull, this indicates that the translation is being traced. It
> + * points to the list of oftrace nodes to which the translation should
> add
> + * tracing information (with oftrace_node_append()). */
> + struct ovs_list *trace;
I don't believe there is a function called oftrace_node_append(). Did you mean
oftrace_report()?
Thanks for working on this. The output is much more readable now.
Acked-by: Justin Pettit <jpet...@ovn.org>
--Justin
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
