[ovs-dev] [PATCH ovn v6 2/9] ovn-controller: Implement translation of OVN flows into OpenFlow.

Justin Pettit jpettit at nicira.com
Tue May 5 00:22:39 UTC 2015


> On May 1, 2015, at 4:17 PM, Ben Pfaff <blp at 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





More information about the dev mailing list