That answers all my questions. I was indeed in the 1.10 branch. (And please reformat the C code. It shares snippets with the sFlow decoder in Ganglia, but not enough to matter.)
Neil On Mar 31, 2013, at 8:31 PM, Ben Pfaff <b...@nicira.com> wrote: > 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
