On 12 September 2015 at 16:43, Justin Pettit <jpet...@nicira.com> wrote:
>
> > On Aug 27, 2015, at 11:21 PM, Alex Wang <ee07b...@gmail.com> wrote:
> >
> >
> > --- a/ovn/controller-vtep/binding.c
> > +++ b/ovn/controller-vtep/binding.c
> > @@ -226,7 +226,8 @@ binding_run(struct controller_vtep_ctx *ctx)
> > }
> >
> > /* Removes all port binding association with vtep gateway chassis.
> > - * Returns true when all done. */
> > + * Returns true when done (i.e. there is no change made to 'ovnsb_idl'),
> > + * otherwise returns false. */
>
> I think it might be nice to be explicit that it's 'ctx->ovnsb_idl'.
>
Fixed and added 'ctx->' prefix to all mentioned places,
> > +/* Updates the vtep Logical_Switch table entries' tunnel keys based
> > + * on the port bindings. */
> > +static void
> > +vtep_lswitch_run(struct controller_vtep_ctx *ctx)
> > +{
> > + struct shash vtep_lswitches = SHASH_INITIALIZER(&vtep_lswitches);
> > + struct sset vtep_pswitches = SSET_INITIALIZER(&vtep_pswitches);
> > + struct sset used_ls = SSET_INITIALIZER(&used_ls);
> > + const struct vteprec_physical_switch *pswitch;
> > + const struct sbrec_port_binding *port_binding_rec;
> > + const struct vteprec_logical_switch *vtep_ls;
> > +
> > + /* Registers all vtep physical switch names in the vtep database. */
> > + VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> > + sset_add(&vtep_pswitches, pswitch->name);
> > + }
>
> I think this comment is sort of ambiguous. My first reading was that it
> was storing the switches into the vtep database, which is clearly not
> happening. I think you could just drop the comment, since the variable
> name is good.
>
>
Removed the comments,
> > + const char *pswitch_name = smap_get(&port_binding_rec->options,
> > + "vtep-physical-switch");
> > + const char *lswitch_name = smap_get(&port_binding_rec->options,
> > + "vtep-logical-switch");
>
> It's not worth reworking the patches now, but it's nice to have the
> documentation combined with the code that uses it. For example , these
> options are described in the third patch of this series, but they could
> have been included in this one.
>
>
will do that in future development,
> > +
> > + /* If 'port_binding_rec->chassis' exists then 'pswitch_name'
> > + * and 'lswitch_name' must also exist. */
> > + if (!pswitch_name || !lswitch_name) {
> > + VLOG_ERR("logical port (%s) with no 'options:vtep-physical-"
> > + "switch' or 'options:vtep-logical-switch'
> specified "
> > + "is somehow bound to chassis (%s). this could
> only "
> > + "happen when someone is messing up using
> ovn-sbctl",
>
> I think you can drop "somehow". The last sentence is a bit accusatory. I
> think you may be able to just drop the sentence. You could note in the
> code that it shouldn't happen unless the database has been directly
> modified.
>
>
Dropped the "somehow" and moved the last sentence as comment.
> > +/* Since we do not own any vtep logical switch, just sets their tunnel
> key
> > + * to 0. */
> > +static bool
> > +vtep_lswitch_cleanup(struct ovsdb_idl *vtep_idl)
> > +{
> > + const struct vteprec_logical_switch *vtep_ls;
> > + int64_t tnl_key = 0;
> > + bool done = true;
> > +
> > + VTEPREC_LOGICAL_SWITCH_FOR_EACH (vtep_ls, vtep_idl) {
> > + if (vtep_ls->n_tunnel_key != 1
> > + || vtep_ls->tunnel_key[0] != tnl_key) {
> > + vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
>
> It looks like the tunnel key is optional. This code will set a tunnel key
> even if one wasn't previously set. Is this intentional? I was just
> surprised to see this in a cleanup function.
>
>
I'm also unclear about if this is necessary. The reason I wanted to reset
it, is
because the controller sets it. And since there is not a good way to mark
which
tunnel key is set by controller, I just reset them all.
Maybe we can just remove this function?
> > +/* Updates vtep logical switch tunnel keys. */
> > +void
> > +vtep_run(struct controller_vtep_ctx *ctx)
> > +{
> > + if (!ctx->vtep_idl_txn) {
> > + return;
> > + }
> > + vtep_lswitch_run(ctx);
> > +}
>
> Is there a reason not to fold the logic in vtep_lswitch_run() into this
> function? I see that in the next patch that vtep_cleanup() does more than
> just call vtep_lswitch_cleanup(), but it doesn't look like
> vtep_lswitch_run() gains any logic later.
>
I think I did this after finishing the next patch. And since vtep_run()
changes a lot in the next patch, I just moved it in this patch.
>
> > +bool
> > +vtep_cleanup(struct controller_vtep_ctx *ctx)
> > +{
> > + if (!ctx->vtep_idl_txn) {
> > + return false;
> > + }
> > +
> > + ovsdb_idl_txn_add_comment(ctx->vtep_idl_txn,
> > + "cleans up vtep configuration");
>
> I think it would be more consistent with the other cleanup functions to
> use "cleaning" instead of "cleans". It would be good to prepend this
> comment with "ovn-controller-vtep:" so we know who did it.
>
>
Fixed it,
> Acked-by: Justin Pettit <jpet...@nicira.com>
>
> --Justin
>
>
>
--
Alex Wang,
Open vSwitch developer
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
