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 <ho...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
