Hi Flavio,
Thanks for reviewing. My comments are below.
On Thursday 23 June 2016 10:33 PM, Flaviof wrote:
On Thu, Jun 23, 2016 at 1:05 AM, <bscha...@redhat.com
<mailto:bscha...@redhat.com>> wrote:
From: Russell Bryant <russ...@ovn.org <mailto:russ...@ovn.org>>
+/* Return true if the address sets match, false otherwise. */
+static bool
+address_sets_match(struct address_set *addr_set,
+ const struct sbrec_address_set *addr_set_rec)
- make both parameters const (const struct sbrec_address_set *)
- why not call these addr_set1 and addr_set2 ?
I agree. addr_set1 and addr_set2 is more readable.
+static void
+update_address_sets(struct controller_ctx *ctx)
+{
+ /* Remember the names of all address sets currently in
expr_address_sets
+ * so we can detect address sets that have been deleted. */
+ struct sset cur_address_sets =
SSET_INITIALIZER(&cur_address_sets);
This sset is not an address_set, but address_set names (or keys).
How about renaming this variable to cur_address_set_names ?
I agree.
+
+
+/* OVN_Northbound and OVN_Southbound have an identical
Address_Set table.
+ * We always update OVN_Southbound to match the current data in
+ * OVN_Northbound. */
+static void
+sync_address_sets(struct northd_context *ctx)
+{
+ struct shash sb_address_sets =
SHASH_INITIALIZER(&sb_address_sets);
+
+ const struct sbrec_address_set *sb_address_set;
+ SBREC_ADDRESS_SET_FOR_EACH (sb_address_set, ctx->ovnsb_idl) {
+ shash_add(&sb_address_sets, sb_address_set->name,
sb_address_set);
+ }
+
+ const struct nbrec_address_set *nb_address_set;
+ NBREC_ADDRESS_SET_FOR_EACH (nb_address_set, ctx->ovnnb_idl) {
+ sb_address_set = shash_find_and_delete(&sb_address_sets,
+ nb_address_set->name);
+ if (!sb_address_set) {
+ sb_address_set =
sbrec_address_set_insert(ctx->ovnsb_txn);
+ sbrec_address_set_set_name(sb_address_set, nb_address_set->name);
+ }
+
I'm still wrapping my head around ctx and txn but I thought of asking
a dumb question here:
In cases when hash_find_and_delete returns a non-null sb_address_set,
is there a need
to clean anything up before calling sbrec_address_set_set_addresses?
If so, this override may be causing a leak?
I think, shash_find_and_delete returns a db rec. It seems to me that
there is no need for a cleanup. I will confirm it anyways. Thanks for
pointing it out.
+ <table name="Address_Set" title="Address Sets">
+ <p>
+ Each row in this table represents a named set of addresses.
+ An address set may contain MAC, IPv4, or IPv6 addresses.
... and also IPv4 + IPv6 cidr (w.x.y.z/N), right?
Yes. I will update the documentation to include these as well. Thanks.
diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
<http://ovn.at>
index 4f72107..59f9307 100644
--- a/tests/ovn.at <http://ovn.at>
+++ b/tests/ovn.at <http://ovn.at>
@@ -649,6 +649,8 @@ done
ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1234' drop
ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1235 &&
inport == "lp11"' drop
ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1236 &&
outport == "lp33"' drop
+ovn-nbctl create Address_Set name=set1
addresses=\"f0:00:00:00:00:11\",\"f0:00:00:00:00:21\",\"f0:00:00:00:00:31\"
+ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1237 &&
eth.src == $set1 && outport == "lp33"' drop
it would be good to add some more tests to exercise the delete and
update codepaths.
If you think it would help, I could take a stab at them.
I agree. I even saw your pull request. Thanks.
Thank you,
Babu
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
