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

[Ben Pfaff]

On Fri, Mar 18, 2011 at 03:30:49PM -0700, Ethan Jackson wrote:
> Patch looks good to me.  Only one minor comment and a side note.
> Merge when ready.

Thank you.  I think that I will wait for Monday on this one.

> > + ? ?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.

Thanks.  I added a warning.

> > ?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.

I do agree that adding unit tests is a good idea.  It is a long term
goal of mine.