Thank you for the review!
On Mon, May 04, 2015 at 05:22:39PM -0700, Justin Pettit wrote:
>
> > On May 1, 2015, at 4:17 PM, Ben Pfaff <[email protected]> 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.
OK, I added an assertion on this.
> > + 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.
Sure. I added a function ldp_free() and changed the code to use it in
both places.
> > +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.
Hmm, you're right. Removed.
> > +/* 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?
Yes, because there's no other way at this point in the series to see the
flows. Later on in the series when we actually push the flows to the
OpenFlow controller, this gets removed.
> >
> > + /* 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?
It's the same math we use for stubs elsewhere, e.g.:
ofproto/bond.c:329: uint64_t ofpacts_stub[128 / 8];
ofproto/in-band.c:372: uint64_t ofpacts_stub[128 / 8];
ofproto/ofproto-dpif-upcall.c:643: uint64_t
recv_stubs[UPCALL_MAX_BATCH][512 / 8];
ofproto/ofproto-dpif-upcall.c:1404: uint64_t stub[DPIF_FLOW_BUFSIZE / 8];
ofproto/ofproto-dpif-upcall.c:1637: uint64_t slow_path_buf[128 / 8];
ofproto/ofproto-dpif-xlate.c:307: uint64_t action_set_stub[1024 / 8];
ofproto/ofproto-dpif-xlate.c:2808: uint64_t actset_stub[1024 / 8];
ofproto/ofproto-dpif-xlate.c:3135: uint64_t action_list_stub[1024 / 8];
ofproto/ofproto-dpif-xlate.c:3841: uint64_t ofpacts_stub[1024 / 8];
ofproto/ofproto-dpif-xlate.h:65: uint64_t odp_actions_stub[256 / 8];
ofproto/ofproto-dpif.c:1327: uint64_t ofpacts_stub[128 / 8];
ofproto/ofproto.c:3233: uint64_t ofpacts_stub[1024 / 8];
ofproto/ofproto.c:4866: uint64_t ofpacts_stub[1024 / 8];
ofproto/ofproto.c:5519: uint64_t bands_stub[256 / 8];
ofproto/ofproto.c:5601: uint64_t bands_stub[256 / 8];
The idea is that we want a stub that's 64 bytes, but aligned on an
8-byte boundary, and a 64-bit unit is 8 bytes hence the / 8.
> > + 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. :-)
OK, I added a comment at the top of the function:
* We put the Pipeline flows into OpenFlow tables 16 through 47 (inclusive). */
> > + VLOG_INFO("...done");
>
> Same question about the log level.
Same answer ;-)
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
