> On Mar 25, 2016, at 12:04 AM, Darrell Ball <[email protected]> wrote:
Thanks for the patch. I did a quick review of the functionality, but didn't do
a full review yet. There are some style issues that I'd like to see addressed
before I do that. Also, you'll need to fold in a patch like the one at the end
of this message to get this to compile due to recent changes upstream. Can you
look at the current feedback and respin a new patch?
I flagged most of the instances where I saw them, but can you take a pass
through and look for any
- Declare variables in the block that needs them.
- End full sentence comments with a period.
- In general, try to get as many full words into a line as will fit in
79 characters.
The way the patch was sent, "Patch v2: " would be part of the commit message.
Also, can you use "ovn-controller-vtep:" instead of "OVN:" in the title?
> This patch implements BUM support in the VTEP schema.
> This relates to BUM traffic flowing from a gateway towards
> HVs. This code would be relevant to HW gateways and
> the ovs-vtep simulator.
> In order to do this, the mcast macs remote table in the VTEP
> schema is populated based on the OVN SB port binding.
> For each logical switch, the SB port bindings are queried
> to find all the physical locators to send BUM traffic to
> and the VTEP DB is updated.
Can you rejustify these to be closer to 79 characters? If you want to indicate
paragraphs, please insert a blank line.
> These code changes are contained in vtep.c.
>
> An OVN test for the ovs-vtep based gateway was
> enabled for relevant packet types to test this functionality.
> This test is contained in ovn.at
>
> The AUTHORS file was updated as well.
You don't need to mention the source changes in the description, since they'll
be in the commit.
> Signed-off-by: Darrell Ball <[email protected]>
Your signed-off-by should match the address used to submit the patch.
> diff --git a/AUTHORS b/AUTHORS
> index 9e44e4c..dd0fc85 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -50,6 +50,7 @@ Daniel Roman [email protected]
> Daniele Di Proietto [email protected]
> Daniele Venturino [email protected]
> Danny Kukawka [email protected]
> +Darrell Ball [email protected]
Do you want to use this address or your VMware one?
> @@ -84,6 +94,96 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char
> *chassis_ip)
> }
>
>
> +/* Creates a new 'Mcast_Macs_Remote'. entry */
Can you move that period to the end of the sentence?
> +static void
> +vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
> + const char *mac,
I think this line will fit on the previous one.
> +/* Compares prev and new mmr locator sets and return true if they
> + * differ; false otherwise; also preps new locator set
> + * for database write */
> +static bool
> +vtep_process_pls(const struct ovs_list *locators_list,
> + const struct mmr_hash_node_data *mmr_ext,
> + struct vteprec_physical_locator **locators)
It's not immediately obvious what these arguments mean and which is previous
and which is new. Can you describe them in the function description? Also,
can you end full sentences with a period?
> +{
> + int i;
> + size_t n_locators_prev = 0;
> + size_t n_locators_new = list_size(locators_list);
> + struct vtep_rec_physical_locator_list_entry *ploc_entry;
> + const struct vteprec_physical_locator *pl = NULL;
The variables "i" and "pl" don't seem to be used until later code blocks. Can
you move them there?
> + bool prev_and_new_locator_lists_differ = false;
It might be nice to use a shorter name. Maybe "lists_differ"?
> +/* Creates a new 'Mcast_Macs_Remote' entry if needed.
> + * Also cleans prev remote mcast mac entries as needed */
Same comment about ending the sentence with a period.
> /* Maps from ovn logical datapath tunnel key (which is also the vtep
> * logical switch tunnel key) to the corresponding vtep logical switch
> * instance. Also, the shash map 'added_macs' is used for checking
> - * duplicated MAC addresses in the same ovn logical datapath. */
> + * duplicated MAC addresses in the same ovn logical datapath.
> + * mmr_ext is used to track mmr info per LS that needs creation/update
> + * and locators_list collects the physical locators to be bound
> + * for an mmr; physical_locators is used to track existing locators
> + * and filter duplicates. */
Can you use single quotes around the field names?
>
> @@ -222,18 +336,40 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct shash *ucast_macs_rmts,
> continue;
> }
>
> + pl = shash_find_data(physical_locators, chassis_ip);
> + if (!pl) {
> + pl = create_pl(vtep_idl_txn, chassis_ip);
> + shash_add(physical_locators, chassis_ip, pl);
> + }
> + ls_pl = shash_find_data(&ls_node->physical_locators, chassis_ip);
> + if (!ls_pl) {
> + ploc_entry = xmalloc(sizeof *ploc_entry);
> + ploc_entry->vteprec_ploc = pl;
> + list_push_back(&ls_node->locators_list,
> + &ploc_entry->locators_node);
The first character of this line should be right under the ampersand of
"&ls_node".
> + shash_add(&ls_node->physical_locators, chassis_ip, pl);
> + }
> +
> + char *mac_tnlkey =
> + xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
> + ls_node->mmr_ext =
> + shash_find_data(mcast_macs_rmts, mac_tnlkey);
The assignment of these two variables will each fit on one line.
> +
> + if (ls_node->mmr_ext &&
> + ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
> +
> + /* Delete the entry from the hash table so the MMR does
> + * not get removed from the DB later on during stale
> + * checking. */
This comment will fit on two lines.
> @@ -305,17 +443,23 @@ vtep_lswitch_cleanup(struct ovsdb_idl *vtep_idl)
> return done;
> }
>
> -/* Removes all entries in the 'Ucast_Macs_Remote' table in vtep database.
> +/* Removes all entries in the 'Ucast_Macs_Remote'
> + * and 'Mcast_Macs_Remote' table in vtep database.
> * Returns true when all done (no entry to remove). */
> static bool
> vtep_macs_cleanup(struct ovsdb_idl *vtep_idl)
> {
> const struct vteprec_ucast_macs_remote *umr;
> + const struct vteprec_mcast_macs_remote *mmr;
>
> VTEPREC_UCAST_MACS_REMOTE_FOR_EACH (umr, vtep_idl) {
> vteprec_ucast_macs_remote_delete(umr);
> return false;
> }
> + VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, vtep_idl) {
> + vteprec_mcast_macs_remote_delete(mmr);
> + return false;
> + }
This block won't execute until there are no Ucast_Macs_Remote. Do you think
it's worth going through both loops and if either one has an entry and then
return false?
> @@ -338,6 +483,13 @@ vtep_run(struct controller_vtep_ctx *ctx)
> const struct vteprec_ucast_macs_remote *umr;
> const struct vteprec_physical_locator *pl;
> const struct sbrec_port_binding *port_binding_rec;
> + const struct vteprec_mcast_macs_remote *mmr;
> + struct mmr_hash_node_data *mmr_ext;
> + const struct vteprec_physical_locator_set *locator_set;
> + struct vteprec_physical_locator **locators_list;
> + size_t n_locators;
> + size_t i;
> + struct shash_node *node;
Many of these variable appear to only be used in a later nested block. Can you
scope the declarations to where they're needed?
> @@ -360,6 +512,29 @@ vtep_run(struct controller_vtep_ctx *ctx)
> shash_add(&ucast_macs_rmts, mac_ip_tnlkey, umr);
> free(mac_ip_tnlkey);
> }
> + /* Collects 'Mcast_Macs_Remote's. */
> + VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, ctx->vtep_idl) {
> ...
> + locator_set = mmr_ext->mmr->locator_set;
> + n_locators = locator_set->n_locators;
> + locators_list = locator_set->locators;
It looks like these were only introduced to shorten a couple of references
later. I think it would be clearer to drop them and just use
"mmr->locator_set" in the accesses below.
> +
> + shash_init(&mmr_ext->physical_locators);
> + for (i = 0; i < n_locators; i++) {
> + shash_add(&mmr_ext->physical_locators,
> + locators_list[i]->dst_ip,
> + locators_list[i]);
> + }
> +
> + free(mac_tnlkey);
> + }
I'd put a blank line before and after this code block to separate from the
other "Collects..." blocks.
Thanks again.
--Justin
-=-=-=-=-=-=-=-=-=-=-
diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
index 4c7d77e..22eafc5 100644
--- a/ovn/controller-vtep/vtep.c
+++ b/ovn/controller-vtep/vtep.c
@@ -119,7 +119,7 @@ vtep_process_pls(const struct ovs_list *locators_list,
{
int i;
size_t n_locators_prev = 0;
- size_t n_locators_new = list_size(locators_list);
+ size_t n_locators_new = ovs_list_size(locators_list);
struct vtep_rec_physical_locator_list_entry *ploc_entry;
const struct vteprec_physical_locator *pl = NULL;
bool prev_and_new_locator_lists_differ = false;
@@ -159,7 +159,7 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
const struct mmr_hash_node_data *mmr_ext)
{
struct vteprec_physical_locator **locators = NULL;
- size_t n_locators_new = list_size(locators_list);
+ size_t n_locators_new = ovs_list_size(locators_list);
bool mmr_changed = false;
const struct vteprec_physical_locator_set *ploc_set;
@@ -293,7 +293,7 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct sha
ls_node->vtep_ls = vtep_ls;
shash_init(&ls_node->added_macs);
shash_init(&ls_node->physical_locators);
- list_init(&ls_node->locators_list);
+ ovs_list_init(&ls_node->locators_list);
ls_node->mmr_ext = NULL;
hmap_insert(&ls_map, &ls_node->hmap_node,
hash_uint64((uint64_t) vtep_ls->tunnel_key[0]));
@@ -345,7 +345,7 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct sha
if (!ls_pl) {
ploc_entry = xmalloc(sizeof *ploc_entry);
ploc_entry->vteprec_ploc = pl;
- list_push_back(&ls_node->locators_list,
+ ovs_list_push_back(&ls_node->locators_list,
&ploc_entry->locators_node);
shash_add(&ls_node->physical_locators, chassis_ip, pl);
}
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
