On Wed, Nov 30, 2011 at 12:16:08PM -0800, Pravin B Shelar wrote:
> Following patch fixes memory leak in case there is ODP_FIT_ERROR
> on flow key.

The changes that match the summary look good to me.  Thank you, those
are good catches.

This one I'm less sure about:

> @@ -2788,6 +2791,14 @@ update_stats(struct ofproto_dpif *p)
>  
>          fitness = odp_flow_key_to_flow(key, key_len, &flow);
>          if (fitness == ODP_FIT_ERROR) {
> +            struct ds s;
> +
> +            ds_init(&s);
> +            odp_flow_key_format(key, key_len, &s);
> +            VLOG_ERR_RL(&rl, "unexpected flow from datapath %s", 
> ds_cstr(&s));
> +            ds_destroy(&s);
> +
> +            dpif_flow_del(p->dpif, key, key_len, NULL);
>              continue;
>          }

It's much louder than the similar case in the "else" clause at the end
of the while loop.  I think that these two cases should be treated in a
similar manner; both simply mean that there is a flow that we don't
expect in the datapath.  I think that your new code is probably what we
really want these days.  The appropriate log level looks to me like WARN
("A low-level operation failed, but higher-level subsystems may be able
to recover.").

It seems to me that this should be broken into two patches.

Thanks,

Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to