----- Original Message -----
> From: "Justin Pettit" <jpet...@ovn.org>
> To: "Lance Richardson" <lrich...@redhat.com>
> Cc: dev@openvswitch.org
> Sent: Thursday, July 7, 2016 5:25:52 PM
> Subject: Re: [ovs-dev] [PATCH] ovn-controller: eliminate stall in ofctrl
> state machine
>
>
> > On Jul 5, 2016, at 5:58 AM, Lance Richardson <lrich...@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
>
>
>
Hi Justin,
Thanks for taking a look.
An infinite loop could be created with the current code by modifying
the state machine, but such a loop would have to involve only "run" events.
It seems very unlikely, but there is the possibility of a loop with this patch
through S_NEW->S_TLV_TABLE_REQUESTED->S_TLV_TABLE_MOD_SENT->S_NEW. I think
it's worth guarding against such a possibility.
I'll post a v2 with a limit on the maximum number of state transitions
per call, assuming that would be acceptable.
Thanks again,
Lance
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
