[ovs-dev] [PATCH] dpif-netdev: removed hmap flow_table

[Ben Pfaff]

On Tue, May 13, 2014 at 04:15:18PM -0700, Daniele Di Proietto wrote:
> As suggested by others, we can use the classifier, instead of the
> hash table, as the only flow container in dpif-netdev
> 
> Signed-off-by: Daniele Di Proietto <ddiproietto at vmware.com>

This needs {}:
> +    if (!netdev_flow)
> +        return NULL;

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");
>      }

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.