[ovs-dev] [overload 1/7] ofproto: Avoid wasting memory malloc()'ing empty action sets for subrules.

Justin Pettit jpettit at nicira.com
Fri Oct 1 22:19:24 UTC 2010


This looks like a good improvement.  I'm a bit concerned about code that may not be defensively written to protect against "rule->actions" being NULL.  For example the following call in flow_stats_ds_cb():

	ofp_print_actions(results, &rule->actions->header, act_len);

This isn't currently a problem due to a check earlier in the function for whether it's a subrule.  However, the check isn't about whether "rule->n_actions" is 0, so it seems like it wouldn't be hard to accidentally introduce a segfault.

--Justin


On Sep 30, 2010, at 5:17 PM, Ben Pfaff wrote:

> GNU libc treats malloc(0) as malloc(1).  Subrules always have an n_actions
> of 0, so this code was wasting time and memory for subrules.  This commit
> stops doing that.
> ---
> ofproto/ofproto.c |    6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index e571bd4..ee05fdd 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1863,8 +1863,10 @@ rule_create(struct ofproto *ofproto, struct rule *super,
>     } else {
>         list_init(&rule->list);
>     }
> -    rule->n_actions = n_actions;
> -    rule->actions = xmemdup(actions, n_actions * sizeof *actions);
> +    if (n_actions > 0) {
> +        rule->n_actions = n_actions;
> +        rule->actions = xmemdup(actions, n_actions * sizeof *actions);
> +    }
>     netflow_flow_clear(&rule->nf_flow);
>     netflow_flow_update_time(ofproto->netflow, &rule->nf_flow, rule->created);
> 
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org





More information about the dev mailing list