[ovs-dev] [PATCH v3 10/41] ofproto: Handle monitor and delete commands in flow monitor requests

[Ben Pfaff]

On Wed, Jun 25, 2014 at 06:22:30PM +0900, Simon Horman wrote:
> On Mon, Jun 23, 2014 at 05:14:52PM -0700, Ben Pfaff wrote:
> > On Mon, Jun 16, 2014 at 11:29:30AM +0900, Simon Horman wrote:
> > > Handle modify and delete commands in OpenFlow1.4 flow monitor requests.
> > > These commands are not yet allowed by the decoder which
> > > will be updated by a subsequent patch.
> > > 
> > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > > 
> > > ---
> > > v2
> > > * No change
> > > ---
> > >  ofproto/ofproto.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > > index 3153e71..211f45e 100644
> > > --- a/ofproto/ofproto.c
> > > +++ b/ofproto/ofproto.c
> > > @@ -4880,12 +4880,23 @@ handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
> > >              goto error;
> > >          }
> > >  
> > > -        error = ofmonitor_create(&request, ofconn, &m);
> > > -        if (error) {
> > > -            goto error;
> > > +        if (request.command == OFPFMC14_DELETE ||
> > > +            request.command == OFPFMC14_MODIFY) {
> > > +            error = flow_monitor_delete(ofconn, request.id);
> > > +            if (error) {
> > > +                goto error;
> > > +            }
> > >          }
> > >  
> > > -        list_insert(&monitor_list, &m->list_node);
> > > +        if (request.command == OFPFMC14_ADD ||
> > > +            request.command == OFPFMC14_MODIFY) {
> > > +            error = ofmonitor_create(&request, ofconn, &m);
> > > +            if (error) {
> > > +                goto error;
> > > +            }
> > > +
> > > +            list_insert(&monitor_list, &m->list_node);
> > > +        }
> > >      }
> > >  
> > >      rule_collection_init(&rules);
> > 
> > I think that this is correct, but this way it is written makes it look
> > like a "modify" could fail in the middle with the monitor deleted and
> > not re-created.  That can't actually happen because ofmonitor_create()
> > only fails if there's a duplicate id.  I'd rather have to code written
> > to be more obviously correct.  (One way to do that would be to have an
> > ofmonitor_modify() function, I guess, but there might be other ways
> > too.)
> 
> Thanks, I understand and agree that is good to make things obvious.
> 
> I've taken a stab at implementing ofmonitor_modify() in the incremental
> change below. Its a bit noisy because of shuffling some existing
> infrastructure around. But would something like this address your concern?

Yeah, I like that, thanks.