> 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