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 <et...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
