On Wed, Nov 23, 2011 at 03:29:57PM -0800, Ben Pfaff wrote:
> On Wed, Nov 23, 2011 at 03:14:05PM -0800, Justin Pettit wrote:
> > This commit adds support for tracking the number of packets and bytes
> > sent through a mirror. The numbers are kept in the new "statistics"
> > column on the mirror table in the "tx_packets" and "tx_bytes" keys.
>
> Important stuff
> ---------------
>
> In bridge_configure_mirrors() we need to assign "m->cfg = cfg;" even
> in the case where we found a matching UUID, because IDL copies of
> database records can get destroyed and recreated without the UUID
> changing in corner cases (e.g. database connection goes down and we
> reconnect).
I spotted something else. In compose_output_action__(), this line:
ctx->mirrors = ofport->bundle->mirror_out;
doesn't look right to me.
First, what is 'mirrors' supposed to mean? The comment just says
"Bitmap of associated mirrors." which is pretty vague. I *think* it's
meant to be the mirrors that the translated flow gets sent to. But if
that's the case, then mirror_out is the wrong choice; that has an
entirely different meaning. dst_mirrors is correct.
Second, why = instead of |=? |= seems like the obvious choice to me.
Nothing ever adds src_mirrors to this mask.
output_mirrors() drops mirrors on the basis of VLAN, but it doesn't
update ctx->mirrors.
I think that the correct thing to do for this patch is to drop this
line entirely from compose_output_action__(), then in output_mirrors()
add the line
ctx->mirrors |= m->dup_mirrors;
just above or below the existing line
mirrors &= ~m->dup_mirrors;
I think that has the desired effect of correctly tracking the actual
mirrors to which the flow was output.
But I might be missing something, please check what I'm saying.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
