> 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'.
> --- a/ovn/controller-vtep/gateway.c
> +++ b/ovn/controller-vtep/gateway.c
> @@ -189,7 +189,8 @@ gateway_run(struct controller_vtep_ctx *ctx)
> }
>
> /* Destroys the chassis table entries for vtep physical switches.
> - * Returns true when all done. */
> + * Returns true when done (i.e. there is no change made to 'ovnsb_idl'),
> + * otherwise returns false. */
Same here about 'ctx->ovnsb_idl'.
> +/* 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.
> + 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.
> +
> + /* 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.
> +/* 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.
> +/* 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.
> +/* Cleans up all related entries in vtep. Returns true when done (i.e.
> + * there is no change made to 'vtep_idl'), otherwise returns false. */
Once again, might be good to mention 'ctx->vtep_idl'.
> +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.
Acked-by: Justin Pettit <jpet...@nicira.com>
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
