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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
