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?
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 8891a06..6f8eea3 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -2012,19 +2012,27 @@ static uint64_t monitor_seqno = 1;
COVERAGE_DEFINE(ofmonitor_pause);
COVERAGE_DEFINE(ofmonitor_resume);
-enum ofperr
-ofmonitor_create(const struct ofputil_flow_monitor_request *request,
- struct ofconn *ofconn, struct ofmonitor **monitorp)
+static struct ofmonitor *
+ofmonitor_lookup(struct ofconn *ofconn, uint32_t id)
OVS_REQUIRES(ofproto_mutex)
{
struct ofmonitor *m;
- *monitorp = NULL;
-
- m = ofmonitor_lookup(ofconn, request->id);
- if (m) {
- return OFPERR_OFPMOFC_MONITOR_EXISTS;
+ HMAP_FOR_EACH_IN_BUCKET (m, ofconn_node, hash_int(id, 0),
+ &ofconn->monitors) {
+ if (m->id == id) {
+ return m;
+ }
}
+ return NULL;
+}
+
+static struct ofmonitor *
+ofmonitor_create__(const struct ofputil_flow_monitor_request *request,
+ struct ofconn *ofconn)
+ OVS_REQUIRES(ofproto_mutex)
+{
+ struct ofmonitor *m;
m = xmalloc(sizeof *m);
m->ofconn = ofconn;
@@ -2037,23 +2045,59 @@ ofmonitor_create(const struct
ofputil_flow_monitor_request *request,
minimatch_init(&m->match, &request->match);
list_init(&m->list_node);
- *monitorp = m;
+ return m;
+}
+
+enum ofperr
+ofmonitor_create(const struct ofputil_flow_monitor_request *request,
+ struct ofconn *ofconn, struct ofmonitor **monitorp)
+ OVS_REQUIRES(ofproto_mutex)
+{
+ struct ofmonitor *m;
+
+ *monitorp = NULL;
+
+ m = ofmonitor_lookup(ofconn, request->id);
+ if (m) {
+ return OFPERR_OFPMOFC_MONITOR_EXISTS;
+ }
+
+ *monitorp = ofmonitor_create__(request, ofconn);
return 0;
}
-struct ofmonitor *
-ofmonitor_lookup(struct ofconn *ofconn, uint32_t id)
+enum ofperr
+ofmonitor_modify(const struct ofputil_flow_monitor_request *request,
+ struct ofconn *ofconn, struct ofmonitor **monitorp)
+ OVS_REQUIRES(ofproto_mutex)
+{
+ enum ofperr error;
+
+ error = ofmonitor_delete(request->id, ofconn);
+ if (error) {
+ return error;
+ }
+
+ *monitorp = ofmonitor_create__(request, ofconn);
+ return 0;
+}
+
+enum ofperr
+ofmonitor_delete(uint32_t id, struct ofconn *ofconn)
OVS_REQUIRES(ofproto_mutex)
{
struct ofmonitor *m;
+ enum ofperr error;
- HMAP_FOR_EACH_IN_BUCKET (m, ofconn_node, hash_int(id, 0),
- &ofconn->monitors) {
- if (m->id == id) {
- return m;
- }
+ m = ofmonitor_lookup(ofconn, id);
+ if (m) {
+ ofmonitor_destroy(m);
+ error = 0;
+ } else {
+ error = OFPERR_OFPMOFC_UNKNOWN_MONITOR;
}
- return NULL;
+
+ return error;
}
void
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index df024e6..453163b 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -213,10 +213,13 @@ struct ofmonitor {
struct ofputil_flow_monitor_request;
-enum ofperr ofmonitor_create(const struct ofputil_flow_monitor_request *,
+enum ofperr ofmonitor_create(const struct ofputil_flow_monitor_request
*request,
struct ofconn *, struct ofmonitor **)
OVS_REQUIRES(ofproto_mutex);
-struct ofmonitor *ofmonitor_lookup(struct ofconn *, uint32_t id)
+enum ofperr ofmonitor_modify(const struct ofputil_flow_monitor_request
*request,
+ struct ofconn *, struct ofmonitor **)
+ OVS_REQUIRES(ofproto_mutex);
+enum ofperr ofmonitor_delete(uint32_t id, struct ofconn *)
OVS_REQUIRES(ofproto_mutex);
void ofmonitor_destroy(struct ofmonitor *)
OVS_REQUIRES(ofproto_mutex);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b90abb5..8121b32 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4915,24 +4915,6 @@ ofmonitor_collect_resume_rules(struct ofmonitor *m,
}
static enum ofperr
-flow_monitor_delete(struct ofconn *ofconn, uint32_t id)
- OVS_REQUIRES(ofproto_mutex)
-{
- struct ofmonitor *m;
- enum ofperr error;
-
- m = ofmonitor_lookup(ofconn, id);
- if (m) {
- ofmonitor_destroy(m);
- error = 0;
- } else {
- error = OFPERR_OFPMOFC_UNKNOWN_MONITOR;
- }
-
- return error;
-}
-
-static enum ofperr
handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
OVS_EXCLUDED(ofproto_mutex)
{
@@ -4966,21 +4948,29 @@ handle_flow_monitor_request(struct ofconn *ofconn,
const struct ofp_header *oh)
goto error;
}
- if (request.command == OFPFMC14_DELETE ||
- request.command == OFPFMC14_MODIFY) {
- error = flow_monitor_delete(ofconn, request.id);
- if (error) {
- goto error;
- }
+ m = NULL;
+ switch (request.command) {
+ case OFPFMC14_ADD:
+ error = ofmonitor_create(&request, ofconn, &m);
+ break;
+
+ case OFPFMC14_MODIFY:
+ error = ofmonitor_modify(&request, ofconn, &m);
+ break;
+
+ case OFPFMC14_DELETE:
+ error = ofmonitor_delete(request.id, ofconn);
+ break;
+
+ default:
+ OVS_NOT_REACHED();
}
- if (request.command == OFPFMC14_ADD ||
- request.command == OFPFMC14_MODIFY) {
- error = ofmonitor_create(&request, ofconn, &m);
- if (error) {
- goto error;
- }
+ if (error) {
+ goto error;
+ }
+ if (m) {
list_push_back(&monitor_list, &m->list_node);
}
}
@@ -5022,7 +5012,7 @@ handle_flow_monitor_cancel(struct ofconn *ofconn, const
struct ofp_header *oh)
id = ofputil_decode_flow_monitor_cancel(oh);
ovs_mutex_lock(&ofproto_mutex);
- error = flow_monitor_delete(ofconn, id);
+ error = ofmonitor_delete(id, ofconn);
ovs_mutex_unlock(&ofproto_mutex);
return error;
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
