On reflection, it's better if the sflow-test.c file just has the same Apache
license as the other files in the tests directory. So I think the header
should just be:
/*
* Copyright (c) 2011, 2012 Nicira, Inc.
* Copyright (c) 2013 InMon Corp.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at:
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
You don't need me sign off again, right? You can submit the revised patch
yourself?
Neil
On Mar 31, 2013, at 9:48 PM, Neil McKee wrote:
> 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
