[ovs-dev] [PATCH] ovn-controller: eliminate stall in ofctrl state machine

[Justin Pettit]

> On Jul 5, 2016, at 5:58 AM, Lance Richardson <lrichard at redhat.com> wrote:
> 
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -410,36 +410,36 @@ ofctrl_run(const struct ovsrec_bridge *br_int)
>         default:
>             OVS_NOT_REACHED();
>         }
> -    } while (state != old_state);
> 
> -    for (int i = 0; state == old_state && i < 50; i++) {
> -        struct ofpbuf *msg = rconn_recv(swconn);
> -        if (!msg) {
> -            break;
> -        }
> +        for (int i = 0; state == old_state && i < 50; i++) {
> +            struct ofpbuf *msg = rconn_recv(swconn);
> +            if (!msg) {
> +                break;
> +            }
> 
> -        const struct ofp_header *oh = msg->data;
> -        enum ofptype type;
> -        enum ofperr error;
> +            const struct ofp_header *oh = msg->data;
> +            enum ofptype type;
> +            enum ofperr error;
> 
> -        error = ofptype_decode(&type, oh);
> -        if (!error) {
> -            switch (state) {
> +            error = ofptype_decode(&type, oh);
> +            if (!error) {
> +                switch (state) {
> #define STATE(NAME) case NAME: recv_##NAME(oh, type); break;
> -                STATES
> +                    STATES
> #undef STATE
> -            default:
> -                OVS_NOT_REACHED();
> +                default:
> +                    OVS_NOT_REACHED();
> +                }
> +            } else {
> +                char *s = ofp_to_string(oh, ntohs(oh->length), 1);
> +                VLOG_WARN("could not decode OpenFlow message (%s): %s",
> +                          ofperr_to_string(error), s);
> +                free(s);
>             }
> -        } else {
> -            char *s = ofp_to_string(oh, ntohs(oh->length), 1);
> -            VLOG_WARN("could not decode OpenFlow message (%s): %s",
> -                      ofperr_to_string(error), s);
> -            free(s);
> -        }
> 
> -        ofpbuf_delete(msg);
> -    }
> +            ofpbuf_delete(msg);
> +        }
> +    } while (state != old_state);

Thanks for tracking this down.  My one concern is that I think this subtly changes the existing logic to prevent loops.  If the recv state machine is constantly changing states, it will never break out of this loop, when before it would cause the function to return.  I don't think the existing recv state machine can do that, but modifying the receive state machine could easily introduce a loop.

What do you think?

--Justin