[ovs-dev] [xlate v1 02/18] ofproto-dpif: Modularize mirror code.

[Ben Pfaff]

On Mon, Jun 24, 2013 at 06:59:16PM -0700, Ethan Jackson wrote:
> This code modularizes ofproto-dpif's mirror code by moving it to
> ofproto-dpif-mirror.  Not only does this shorten ofproto-dpif and
> hide complexity, but its also necessary for future patches which
> modularize ofproto-dpif-xlate in preparation for multi-threading.
> 
> Signed-off-by: Ethan Jackson <ethan at nicira.com>

"git am" says:

    Applying: ofproto-dpif: Modularize mirror code.
    /home/blp/ovs/.git/rebase-apply/patch:380: trailing whitespace.
     * 'mirrors'.  Returns true if such a mirror exists, false otherwise. 
    warning: 1 line adds whitespace errors.

It seems a little odd to make the mirror code keep a map from
ofproto_dpif to mbridge, instead of making ofproto_dpif keep an
mbridge pointer.  I think you might have mentioned the reason to me in
person, but I don't remember what it was.

It seems like the direct cost/benefit ratio for this change is low.
There's a lot of various kinds of extra overhead (memory, time, code)
for relatively little benefit.  That's too bad.  I hope that the later
modularization makes it worth it.

The interface for mirror_get_ffs() is really strange.  I would have
the caller pass in a mirror index, not a bitmask from which the
function extracts the lowest 1-bit.

Please don't use ovs_assert() on an expression that must be evaluated.
It's true that ovs_assert() cannot be disabled currently, but I'd like
to leave open that possibility later.

add_mirror_actions() leaves me a little unsatisfied.  It seems to me
that one could write a function of the form

    mirror_mask_t
    mirror_get_outputs(const struct ofproto_dpif *,
                       mirror_mask mirrors,
                       struct of_bundle *out_bundles[MAX_MIRRORS + 1],
                       int out_vlans[MAX_MIRRORS + 1],
                       struct flow_wildcards *wc);

that would go through 'mirrors', figure out which ones should really
be output to, populate out_bundles with the bundles for output
(null-terminated?), out_vlans with the vlans for output (-1
terminated?), update 'wc', and return the dup_mirrors.  That would
make add_mirror_actions() at least marginally simpler.

In run(), it's a little odd to bother making mac_learning_flush()
report the invalidated tags, since we're doing a full need_revalidate
anyway.  I see that the original code did the same; I didn't notice
that it was odd before.