[ovs-dev] [cfm 3/3] ofproto: Take responsibility for configuration fault management (CFM).

[Ethan Jackson]

Patch looks good to me.  Only one minor comment and a side note.
Merge when ready.

Ethan

> +    cfm_update_remote_mps(ofport->cfm, remote_mps, n_remote_mps);
> +
> +    if (!cfm_configure(ofport->cfm)) {
> +        cfm_destroy(ofport->cfm);
> +        ofport->cfm = NULL;
> +    }

It may be worth warning here.

>  static struct ofpbuf *
>  xlate_actions(struct action_xlate_ctx *ctx,
>               const union ofp_action *in, size_t n_in)
> @@ -3183,13 +3290,18 @@ xlate_actions(struct action_xlate_ctx *ctx,
>     ctx->recurse = 0;
>     ctx->last_pop_priority = -1;
>
> -    if (!ctx->check_special
> -        || !ctx->ofproto->ofhooks->special_cb
> -        || ctx->ofproto->ofhooks->special_cb(&ctx->flow, ctx->packet,
> -                                             ctx->ofproto->aux)) {
> -        do_xlate_actions(in, n_in, ctx);
> -    } else {
> +    if (ctx->check_special && cfm_should_process_flow(&ctx->flow)) {
> +        if (ctx->packet) {
> +            ofproto_process_cfm(ctx->ofproto, &ctx->flow, ctx->packet);
> +        }
>         ctx->may_set_up_flow = false;
> +    } else if (ctx->check_special
> +               && ctx->ofproto->ofhooks->special_cb
> +               && !ctx->ofproto->ofhooks->special_cb(&ctx->flow, ctx->packet,
> +                                                     ctx->ofproto->aux)) {
> +        ctx->may_set_up_flow = false;
> +    } else {
> +        do_xlate_actions(in, n_in, ctx);
>     }

xlate actions seems to be less and less appropriately named. I think
once you merge this patch I may look into splitting the special packet
processing stuff into it's own function.  It would be nice if we could
remove as many of the side effects in this function as possible.  We
could possibly add some unit tests if successful in this endeavor.
Just a side note, I think what's written here is good.