> On May 1, 2015, at 4:17 PM, Ben Pfaff <b...@nicira.com> wrote:
> +/* Creates a new logical_datapath with the given 'uuid'. */
> +static struct logical_datapath *
> +ldp_create(const struct uuid *uuid)
> +{
> + static uint32_t next_integer = 1;
> + struct logical_datapath *ldp;
> +
> + ldp = xmalloc(sizeof *ldp);
> + hmap_insert(&logical_datapaths, &ldp->hmap_node, uuid_hash(uuid));
> + ldp->uuid = *uuid;
> + ldp->integer = next_integer++;
It's unlikely that we'll wrap without a bug or malicious user, but, if it does,
I think it could lead to leaking between logical datapath. If you don't want
to track all the used values, we could at least put an assert if 0 is hit.
> + struct logical_datapath *next_ldp;
> + HMAP_FOR_EACH_SAFE (ldp, next_ldp, hmap_node, &logical_datapaths) {
> + if (simap_is_empty(&ldp->ports)) {
> + simap_destroy(&ldp->ports);
> + hmap_remove(&logical_datapaths, &ldp->hmap_node);
> + free(ldp);
> + }
> + }
> +}
> +
> +static void
> +ldp_destroy(void)
> +{
> + struct logical_datapath *ldp, *next_ldp;
> + HMAP_FOR_EACH_SAFE (ldp, next_ldp, hmap_node, &logical_datapaths) {
> + simap_destroy(&ldp->ports);
> + hmap_remove(&logical_datapaths, &ldp->hmap_node);
> + free(ldp);
> + }
> +}
Very minor, but do you think it's worth creating a destroy function for an ldp
record? I always worry that someone adds a field with dynamic data, but
forgets to free it in one location.
> +void
> +pipeline_init(struct controller_ctx *ctx)
> +{
> + symtab_init();
> +
> + ovsdb_idl_add_column(ctx->ovnsb_idl,
> &sbrec_pipeline_col_logical_datapath);
> + ovsdb_idl_add_column(ctx->ovnsb_idl, &sbrec_pipeline_col_table_id);
> + ovsdb_idl_add_column(ctx->ovnsb_idl, &sbrec_pipeline_col_priority);
> + ovsdb_idl_add_column(ctx->ovnsb_idl, &sbrec_pipeline_col_match);
> + ovsdb_idl_add_column(ctx->ovnsb_idl, &sbrec_pipeline_col_actions);
I think we monitor everything by default in the SB IDL, so these are probably
unnecessary.
> +/* Translates logical flows in the Pipeline table in the OVN_SB database
> + * into OpenFlow flows. */
> +void
> +pipeline_run(struct controller_ctx *ctx)
> +{
> + struct hmap flows = HMAP_INITIALIZER(&flows);
> + uint32_t conj_id_ofs = 1;
> +
> + ldp_run(ctx);
> +
> + VLOG_INFO("starting run...");
Do you want to log this at level info?
>
> + /* Translate OVN actions into OpenFlow actions. */
> + uint64_t ofpacts_stub[64 / 8];
The math for the size of this array seems mysterious. Would it make sense to
either specify the size directly or use #define names?
> + struct ofpbuf ofpacts;
> + struct expr *prereqs;
> + char *error;
> +
> + ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
> + error = actions_parse_string(pipeline->actions, &symtab, &ldp->ports,
> + pipeline->table_id + 16,
I still think it would be helpful to document how tables are being used. :-)
> + VLOG_INFO("...done");
Same question about the log level.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
