[ovs-dev] [PATCH 08/23] ovn-controller: Factor patch port management into new "patch" module.

Justin Pettit jpettit at vmware.com
Thu Oct 15 23:38:34 UTC 2015


After reviewing patch 10, I have a few more thoughts on this patch.  This doesn't change my original ACK, though.

> +static void
> +create_patch_port(struct controller_ctx *ctx,
> +                  const char *network,
> +                  const struct ovsrec_bridge *b1,
> +                  const struct ovsrec_bridge *b2)
> +{

This could use a comment and maybe a renaming of some arguments.  In particular, it isn't obvious the importance between "b1" and "b2".  Also, it might be worth noting that it only creates one side of the patch port.

> +static void
> +create_patch_ports(struct controller_ctx *ctx,
> +                   const char *network,
> +                   struct shash *existing_ports,
> +                   const struct ovsrec_bridge *b1,
> +                   const struct ovsrec_bridge *b2)

This could use a comment, since many of the arguments are non-obvious.  In particular, how "existing_ports" will be modified and the difference between "b1" and "b2".

> +static void
> +parse_bridge_mappings(struct controller_ctx *ctx,
> +                      const struct ovsrec_bridge *br_int,
> +                      const char *mappings_cfg)

This function does a lot more than just parse the bridge mappings.  It could probably use a better name, but, at the very least, I think a comment would be helpful--especially for the non-obvious things like what will happen to "mappings_cfg".

--Justin





More information about the dev mailing list