Thanks for the review;
The suggestions are fine;
I added a couple comments inline.
Darrell
On Mon, Apr 25, 2016 at 3:44 PM, Justin Pettit <jpet...@ovn.org> wrote:
>
> > On Apr 5, 2016, at 1:13 PM, Darrell Ball <dlu...@gmail.com> wrote:
> >
> > 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.
> >
> > Some test packets were enabled in the HW gateway test case to exercise
> the
> > new code.
>
> I told you to limit the line length to 80 columns, but when you run "git
> log", it indents the message by four spaces, so it looks like better advice
> would be 75 or 76 columns.
>
acked
>
> The name of the patch shouldn't have "Patch v3:" be part of the title.
>
acked
>
> > diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
> > index 016c2e0..aa988c0 100644
> > --- a/ovn/controller-vtep/vtep.c
> > +++ b/ovn/controller-vtep/vtep.c
> > @@ -27,9 +27,20 @@
> > #include "openvswitch/vlog.h"
> > #include "ovn/lib/ovn-sb-idl.h"
> > #include "vtep/vtep-idl.h"
> > +#include "openvswitch/util.h"
>
> Is this additional #include needed? It seems like builds fine without it.
>
> > + if (n_locators_new) {
> > + int i = 0;
> > + struct vtep_rec_physical_locator_list_entry *ploc_entry;
> > + const struct vteprec_physical_locator *pl;
> > + LIST_FOR_EACH (ploc_entry, locators_node, locators_list) {
> > + locators[i] = (struct vteprec_physical_locator *)
> > + ploc_entry->vteprec_ploc;
> > + if(mmr_ext) {
> > + pl = shash_find_data(&mmr_ext->physical_locators,
> > + locators[i]->dst_ip);
> > + if (!pl) {
> > + locator_lists_differ = true;
> > + }
>
> I think the code can be slightly simplified since "pl" is never used other
> than to see if's not null like so:
>
Its not as ugly compressed as I had originally thought :-)
>
> LIST_FOR_EACH (ploc_entry, locators_node, locators_list) {
> locators[i] = (struct vteprec_physical_locator *)
> ploc_entry->vteprec_ploc;
> if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
> locators[i]->dst_ip)) {
> locator_lists_differ = true;
> }
>
> > +/* Creates a new 'Mcast_Macs_Remote' entry. if needed and also cleans up
> > + * out-dated remote mcast mac entries as needed. */
> > +static void
> > +vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
> > + struct ovs_list *locators_list,
> > + const struct vteprec_logical_switch *vtep_ls,
> > + const struct mmr_hash_node_data *mmr_ext)
> > +{
> > + struct vteprec_physical_locator **locators = NULL;
> > + size_t n_locators_new = ovs_list_size(locators_list);
> > + bool mmr_changed = false;
> > +
> > + locators = xmalloc(n_locators_new * sizeof *locators);
> > +
> > + if (vtep_process_pls(locators_list, mmr_ext, locators)) {
> > + mmr_changed = true;
> > + }
>
> I don't think you need to initialize "mmr_changed" or have this "if"
> block, since vtep_process_pls() always returns true or false.
>
thanks for catching this one
>
> > -/* Updates the vtep 'Ucast_Macs_Remote' table based on non-vtep port
> > - * bindings. */
> > +/* Updates the vtep 'Ucast_Macs_Remote' and 'Mcast_Macs_Remote' tables
> based
> > + * on non-vtep port bindings. */
> > static void
> > vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash
> *ucast_macs_rmts,
> > - struct shash *physical_locators, struct shash *vt We
> sep_lswitches,
> > - struct shash *non_vtep_pbs)
> > + struct shash *mcast_macs_rmts, struct shash
> *physical_locators,
> > + struct shash *vtep_lswitches, struct shash *non_vtep_pbs)
> > {
> > struct shash_node *node;
> > struct hmap ls_map;
> > + const struct vteprec_physical_locator *pl;
>
> I believe this can be moved to a more speciifc code block.
>
yes, it slipped though the cracks
>
> > + /* Collects 'Mcast_Macs_Remote's. */
> > + VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, ctx->vtep_idl) {
> > + struct mmr_hash_node_data *mmr_ext = xmalloc(sizeof *mmr_ext);;
> > + char *mac_tnlkey =
> > + xasprintf("%s_%"PRId64, mmr->MAC,
> > + mmr->logical_switch &&
> mmr->logical_switch->n_tunnel_key
> > + ? mmr->logical_switch->tunnel_key[0] :
> INT64_MAX);
> > +
> > + shash_add(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
> > + mmr_ext->mmr = mmr;
> > +
> > + shash_init(&mmr_ext->physical_locators);
> > + for (size_t i = 0; i < mmr_ext->mmr->locator_set->n_locators;
> i++) {
> > + shash_add(&mmr_ext->physical_locators,
> > + mmr_ext->mmr->locator_set->locators[i]->dst_ip,
> > + mmr_ext->mmr->locator_set->locators[i]);
>
> Minor, but I think it's clearer if you just access these through "mmr"
> rather than "mmr_ext->mmr".
>
yes, reduce one indirection
>
> I made a few other minor style changes. If you agree with all the changes
> at the end of this message, I'll go ahead and apply the patch with those
> changes.
>
> Thanks,
>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
>
> diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
> index aa988c0..9e11e28 100644
> --- a/ovn/controller-vtep/vtep.c
> +++ b/ovn/controller-vtep/vtep.c
> @@ -27,7 +27,6 @@
> #include "openvswitch/vlog.h"
> #include "ovn/lib/ovn-sb-idl.h"
> #include "vtep/vtep-idl.h"
> -#include "openvswitch/util.h"
>
> VLOG_DEFINE_THIS_MODULE(vtep);
>
> @@ -93,9 +92,8 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char
> *chas
>
> return new_pl;
> }
> -
> ^L
> -/* Creates a new 'Mcast_Macs_Remote' entry. */
> +/* Creates a new 'Mcast_Macs_Remote'. */
> static void
> vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
> const struct vteprec_logical_switch *vtep_ls,
> @@ -109,14 +107,13 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
> const
> vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set);
> }
>
> -/* Compares prev and new mmr locator sets and return true if they differ
> and
> - * false otherwise. This function also preps new locator set for database
> - * write.
> +/* Compares previous and new mmr locator sets and returns true if they
> + * differ and false otherwise. This function also preps a new locator
> + * set for database write.
> *
> - * locators_list is the new set of locators for the associated
> - * 'Mcast_Macs_Remote' entry passed in and is queried to generate the new
> set
> - * of locators in vtep database format.
> - */
> + * 'locators_list' is the new set of locators for the associated
> + * 'Mcast_Macs_Remote' entry passed in and is queried to generate the
> + * new set of locators in vtep database format. */
> static bool
> vtep_process_pls(const struct ovs_list *locators_list,
> const struct mmr_hash_node_data *mmr_ext,
> @@ -136,16 +133,12 @@ vtep_process_pls(const struct ovs_list
> *locators_list,
> if (n_locators_new) {
> int i = 0;
> struct vtep_rec_physical_locator_list_entry *ploc_entry;
> - const struct vteprec_physical_locator *pl;
> LIST_FOR_EACH (ploc_entry, locators_node, locators_list) {
> locators[i] = (struct vteprec_physical_locator *)
> ploc_entry->vteprec_ploc;
> - if(mmr_ext) {
> - pl = shash_find_data(&mmr_ext->physical_locators,
> - locators[i]->dst_ip);
> - if (!pl) {
> + if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
> + locators[i]->dst_ip)) {
> locator_lists_differ = true;
> - }
> }
> i++;
> }
> @@ -154,7 +147,7 @@ vtep_process_pls(const struct ovs_list *locators_list,
> return locator_lists_differ;
> }
>
> -/* Creates a new 'Mcast_Macs_Remote' entry. if needed and also cleans up
> +/* Creates a new 'Mcast_Macs_Remote' entry if needed and also cleans up
> * out-dated remote mcast mac entries as needed. */
> static void
> vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
> @@ -164,13 +157,11 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
> {
> struct vteprec_physical_locator **locators = NULL;
> size_t n_locators_new = ovs_list_size(locators_list);
> - bool mmr_changed = false;
> + bool mmr_changed;
>
> locators = xmalloc(n_locators_new * sizeof *locators);
>
> - if (vtep_process_pls(locators_list, mmr_ext, locators)) {
> - mmr_changed = true;
> - }
> + mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators);
>
> if (mmr_ext && !n_locators_new) {
> vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
> @@ -262,7 +253,6 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct sha
> {
> struct shash_node *node;
> struct hmap ls_map;
> - const struct vteprec_physical_locator *pl;
>
> /* Maps from ovn logical datapath tunnel key (which is also the vtep
> * logical switch tunnel key) to the corresponding vtep logical switch
> @@ -338,11 +328,13 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct s
> continue;
> }
>
> - pl = shash_find_data(physical_locators, chassis_ip);
> + const struct vteprec_physical_locator *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);
> }
> +
> const struct vteprec_physical_locator *ls_pl =
> shash_find_data(&ls_node->physical_locators, chassis_ip);
> if (!ls_pl) {
> @@ -420,7 +412,8 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct sha
> free(iter);
> }
> hmap_destroy(&ls_map);
> - /* Clean stale 'mcast_macs_remote' */
> +
> + /* Clean stale 'Mcast_Macs_Remote' */
> struct mmr_hash_node_data *mmr_ext;
> SHASH_FOR_EACH (node, mcast_macs_rmts) {
> mmr_ext = node->data;
> @@ -531,10 +524,10 @@ vtep_run(struct controller_vtep_ctx *ctx)
> mmr_ext->mmr = mmr;
>
> shash_init(&mmr_ext->physical_locators);
> - for (size_t i = 0; i < mmr_ext->mmr->locator_set->n_locators;
> i++) {
> + for (size_t i = 0; i < mmr->locator_set->n_locators; i++) {
> shash_add(&mmr_ext->physical_locators,
> - mmr_ext->mmr->locator_set->locators[i]->dst_ip,
> - mmr_ext->mmr->locator_set->locators[i]);
> + mmr->locator_set->locators[i]->dst_ip,
> + mmr->locator_set->locators[i]);
> }
>
> free(mac_tnlkey);
>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
