[ovs-dev] [PATCH v3 2/2] ovn: Add address_set() support for ACLs.
Babu Shanmugam
bschanmu at redhat.com
Fri Jun 24 10:29:29 UTC 2016
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, <bschanmu at redhat.com
> <mailto:bschanmu at redhat.com>> wrote:
>
> From: Russell Bryant <russell at ovn.org <mailto:russell at 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
More information about the dev
mailing list