On May 16, 2014, at 9:26 AM, Ben Pfaff <b...@nicira.com> wrote:
> This needs {}:
>> + if (!netdev_flow)
>> + return NULL;
>
Sure, i’ll fix it
> VLOG_ERR is pretty strong. ovs-appctl(8) says that it means that "A
> high-level operation or a subsystem failed. Attention is warranted."
> That is, it generally indicates a bug or a failure that might require
> the admin to take a look. Is this such a situation? I can imagine
> races where this might happen, although I guess that they should be rare
> in practice.
>> + if (flow_equal(&netdev_flow->flow, flow)) {
>> + return netdev_flow;
>> + } else {
>> + VLOG_ERR("classifier_lookup returned different flow");
>> }
No problem, I’ll change it to VLOG_WARN, that seems more appropriate.
> I don't think that it is safe to use a cls_cursor to iterate flows in
> the flow dump path. The classifier isn't locked from one
> dpif_netdev_flow_dump_next() to the next, which means that any given
> flow might get deleted from the classifier from one to the next. That
> includes both the most recently returned flow and the next one that will
> be returned. Basically, we have to either somehow prevent at least one
> of those flows from being deleted, or use some other means for
> iteration. That's why the hmap iteration used the indirect
> hmap_at_position() call for iteration: it's always safe, even if it's
> otherwise undesirable
I didn’t think of that. I assumed that a cls_cursor_next() had the same
requirements as hmap_at_position().
I’ve implemented classifier_at_position() to address the issue. I’ll post it
along with a new version of the patch.
Thanks for the review,
Daniele
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
