On Thu, Mar 31, 2016 at 10:05:05AM -0500, Ryan Moats wrote:
> From: RYAN D. MOATS <rmo...@us.ibm.com>
>
> This is a prerequisite for incremental processing.
>
> Signed-off-by: RYAN D. MOATS <rmo...@us.ibm.com>
I'm really happy with the easier-to-understand use of sequence numbers
in this version of the patch. I think that it can be improved further,
by translating the raw "seqno" values into easier-to-understand booleans
that explain whether this is a new or a deleted row, something like
this (ofctrl_add_flow() would need an appropriate change too):
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 041fa66..5d44469 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -213,20 +213,16 @@ add_logical_flows(struct controller_ctx *ctx, const
struct lport_index *lports,
const struct sbrec_logical_flow *lflow;
SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
- unsigned int del_seqno = sbrec_logical_flow_row_get_seqno(lflow,
- OVSDB_IDL_CHANGE_DELETE);
- unsigned int mod_seqno = sbrec_logical_flow_row_get_seqno(lflow,
- OVSDB_IDL_CHANGE_MODIFY);
-
- /* if the row has a del_seqno > 0, then trying to process the
- * row isn't going to work (as it has already been freed).
- * What we can do is to pass a pointer to the ovs_idl_row to
- * ofctrl_remove_flows() to remove flows from this record */
- if (del_seqno > 0) {
+ bool is_deleted = sbrec_logical_flow_row_get_seqno(lflow,
OVSDB_IDL_CHANGE_DELETE) > 0;
+ if (is_deleted) {
+ /* The row is deleted and we can't access it--its column data has
+ * already been freed. Remove it by uuid. */
ofctrl_remove_flows(&lflow->header_.uuid);
continue;
}
+ bool is_new = sbrec_logical_flow_row_get_seqno(lflow,
OVSDB_IDL_CHANGE_MODIFY) == 0;
+
/* Determine translation of logical table IDs to physical table IDs. */
bool ingress = !strcmp(lflow->pipeline, "ingress");
@@ -359,7 +355,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct
lport_index *lports,
}
if (!m->n) {
ofctrl_add_flow(ptable, lflow->priority, &m->match, &ofpacts,
- &lflow->header_.uuid, mod_seqno);
+ &lflow->header_.uuid, is_new);
} else {
uint64_t conj_stubs[64 / 8];
struct ofpbuf conj;
@@ -375,7 +371,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct
lport_index *lports,
dst->n_clauses = src->n_clauses;
}
ofctrl_add_flow(ptable, lflow->priority, &m->match, &conj,
- &lflow->header_.uuid, mod_seqno);
+ &lflow->header_.uuid, is_new);
ofpbuf_uninit(&conj);
}
}
If this is going to be a common use case for seqnos then we might want
some generated IDL helpers, e.g. <table>_row_is_new(),
<table>_row_is_deleted(), etc. to make the code more obvious and to keep
line lengths sane.
I don't understand why ofpacts_hash() isn't just:
uint32_t
ofpacts_hash(const struct ofpact *ofpacts, size_t ofpacts_len, uint32_t
basis)
{
return hash_bytes(ofpacts, ofpacts_len, basis);
}
I get some compiler warnings/errors:
../ovn/controller/binding.c: In function ‘add_local_datapath’:
../ovn/controller/binding.c:136:5: error: implicit declaration of function
‘reset_flow_processing’ [-Werror=implicit-function-declaration]
reset_flow_processing();
^
../ovn/controller/lflow.c: In function ‘lflow_run’:
../ovn/controller/lflow.c:474:9: error: implicit declaration of function
‘ovn_flow_table_clear’ [-Werror=implicit-function-declaration]
ovn_flow_table_clear();
^
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
