| Message ID | 5339F544-24AF-4AB0-AB14-9599458FB71D@ovn.org |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Apr 4, 2016 at 2:50 PM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Mar 25, 2016, at 12:04 AM, Darrell Ball <dlu998@gmail.com> 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. > > Filtered these a couple more times > 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? > sure > > > 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. > > sure > > 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. > ok > > > Signed-off-by: Darrell Ball <dball@vmware.com> > > 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 droman@nicira.com > > Daniele Di Proietto daniele.di.proietto@gmail.com > > Daniele Venturino daniele.venturino@m3s.it > > Danny Kukawka danny.kukawka@bisect.de > > +Darrell Ball dlu998@gmail.com > > Do you want to use this address or your VMware one? > gmail; since several of us are having trouble receiving email from dev @openvswitch when using Vmware email addresses; which is worse recently > > > @@ -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? > ok > > > +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. > ok > > > +/* 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? > added more description > > > +{ > > + 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"? > shortened name > > > +/* 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? > hopefully, I caught most of these > > > > > @@ -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". > thx > > > + 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. > put on same 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. > put on 2 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? > sure; its cleanup but still takes time so made the adjustment > > > @@ -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? > Filtered the file and hopefully caught all these > > > @@ -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. > sure > > > + > > + 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. > added some blank lines where it seemed missing > > 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); > } > > > > > >
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); }