I'll do an update to the patch tomorrow but here's an early response.
On Fri, Mar 29, 2013 at 08:39:42PM -0700, Neil Mckee wrote:
>
> On Mar 29, 2013, at 4:16 PM, Ben Pfaff wrote:
>
> > On Wed, Mar 27, 2013 at 11:02:21PM -0700, Neil Mckee wrote:
> >> This patch adds an sFlow test to the test suite (in branch 1.10). To make
> >> that work properly I added netdev_dummy_get_ifindex() so that a dummy
> >> netdev can return a dummy ifindex when asked. Is there anywhere in OVS
> >> that assumes that a netdev_dummy cannot make up a dummy ifindex? If so, I
> >> guess this behavior could be off by default and turned on just for this
> >> test.
> >>
> >> I have only tested this on a Fedora 17 OS.
> >>
> >> Signed-off-by: Neil McKee <neil.mc...@inmon.com>
> >>
> >> ---
> >
> > Thanks for the test!
> >
> > I noticed that the new test-sflow.c has the Apache 2 boilerplate
> > license text in it, which is fine, but the copyright notice should
> > probably mention InMon in place of or in addition to Nicira, since a
> > lot of the code in test-sflow.c is not written by Nicira. Neil, can
> > you give me a correct copyright notice for that file?
> >
>
> How about we add the same copyright notice that appears at the top of
> all the lib/sflow* source files (just updating the year to 2013)?
OK, that will do, thanks.
> > Running this, I had some trouble with ifindexes. As written, the
> > ifindex that each dummy device receives depends on the order in which
> > they are created. That, in turn, depends on hash order in the
> > database and other factors. I decided to fix it by making the ifindex
> > a configurable property of the dummy devices, applying the following
> > incremental patch. Does that make sense?
>
> Yes. Much better. I had started to do the same thing but baulked at
> the number of lines I was adding.
Thanks.
I think it's OK for the tests to be big.
> > Even after I applied those changes, the test still didn't pass for me.
> > When I looked more closely, some of the expected output didn't
> > entirely make sense. For example, my reading of the sflow spec says
> > that samplePool should more or less count upward, which is what I
> > actually saw in the test output, but the expected output provided in
> > the patch shows all the samplePool values as 0. Some of the other
> > expected output needed to be adjusted too.
> >
>
> I wonder why your samplePool numbers are incrementing and mine are not?
> The samplePool is supplied in ofproto-dpif-sflow.c:dpif-sflow-received():
>
> fs.sample_pool = stats.rx_packets;
>
> So I was assuming that since netdev-dummy doesn't seem to increment
> any of if's interface-counter stats then that would account for
> rx_packets being always 0. Does this vary depending on the presence
> or absence of the kernel module, or something like that?
No, the tests don't ever use the kernel module (they will run whether
or not you compile it), so that does not affect them.
I think the difference is probably the OVS version. The current OVS
"master" version of netdev-dummy properly maintains counters, the
versions on branch-1.10 and earlier do not. I guess that you must have
developed against branch-1.10 or an earlier version.
> I was thinking that we should probably also change "grep HEADER" to
> something like "egrep 'HEADER|ERROR' to make certain than any
> exception flagged in the sflow-test.c code is certain to get through
> and break the test. Same thing for "grep IFCOUNTERS". I hesitated
> because I don't know how portable that egrep construct is.
It should be portable enough as long as we use $EGREP instead of plain
egrep, since the Autoconf manual says:
`egrep'
Posix 1003.1-2001 no longer requires `egrep', but many hosts do
not yet support the Posix replacement `grep -E'. Also, some
traditional implementations do not work on long input lines. To
work around these problems, invoke `AC_PROG_EGREP' and then use
`$EGREP'.
> Just two minor questions left:
>
> 1) I wanted to craft a test packet with an odd-numbered length between
> 64 and 128 bytes, and another with > 128 bytes. Where should I look
> for documentation on that packet-construction language? Can I specify
> something like ipv6-over-vxlan-over-ipv4-over-mpls? I'm not concerned
> about this for now, just thinking about future enhancements to the
> test.
The language isn't really documented, but it doesn't support anything
that complicated anyway.
However, you have another alternative: you can fully specify an Ethernet
frame as a series of hex digits with optional spaces. If you do it that
way, then you can makes the packets as sophisticated as you like.
> 2) Are you really going to let me away with 2-character indentation in C?
I stopped reading that file for style when I realized that it wasn't
really OVS code, it was code that must be cut-and-pasted in from InMon
sflow code, probably sflowtool. At that point, I didn't want to ask for
divergence from whatever upstream source it comes from, because that
will just make life harder for everyone later if we ever need to update
it.
(I know that not complaining about style is out of character for me.)
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
