And yes. I am aware that testing a patch and not covering the actual
problem should not qualify as testing the patch.
My argument for the validity of the patch is that since the new code
touches and treats all packets equally, having it work in general covers
the corner case that was rejected in the original code.
I'll admit that I'm not entirely certain under what circumstances one
would use this behaviour (maybe an environment where multiple hosts
serve the same image?) or if there are Coraid appliances or HBAs that
actually support this.
On 19/05/2014 11:06 PM, Catalin Salgau wrote:
> I'm not exactly sure how this can be tested without relying on a
> second system or packet capture. I'm thinking on the lines of
> attaching vblade to lo0 and writing another BPF based program to make
> requests, but I believe that would not be very reliable under some
> conditions.
>
> Back to the patch. I do not have a testing suite for this. I noticed
> the incorrect behaviour when tweaking the code in another direction.
> I've taken several looks at the BPF filter over the past two years and
> have been running with this change in 'production' for about one, but
> I do not actually use the corner case covered by it. Since I'm
> isolating patches to post to the mailing list, I have tested this
> change against vblade-21 with a full boot-shutdown cycle of Windows 8.
> By reusing the same accumulator for both comparisons, the code is
> slightly shorter(and marginally faster) and this changes a few offsets.
>
> The present code is roughly equivalent to
> if P[16:2] == SHELF:
> if P[18] == SLOT:
> goto ACCEPT
> if P[16:2] != '\xFF\xFF':
> goto REJECT
> if P[18] != '\xFF':
> goto REJECT
>
> The patch changes it to something like
> if P[16:2] == SHELF:
> goto CHECKSLOT
> if P[16:2] != 0xffff:
> goto REJECT
> if P[18] == SLOT:
> goto ACCEPT
> if P[18] != 0xff:
> goto REJECT
>
>
> I would encourage anyone wishing to validate the changes to read the
> FILTER_MACHINE section in bpf(4) in the FreeBSD man pages. Linux LSF
> is supposed to be compatible.
>
>
> On 19/05/2014 10:22 PM, Ed Cashin wrote:
>> I think you mean that a vblade that's AoE 11.22 should answer AoE
>> commands sent to 0xffff.2 or 11.0xff but does not currently.
>>
>> It would be very nice if we had a collection of tests, so that you
>> could also include a test that showed what this fix corrects and
>> validated the fix itself. Alas. Still, it would be good to hear
>> about how the fix was tested if you don't want to take a stab at
>> adding such a test.
>>
>> On May 19, 2014, at 3:11 PM, Catalin Salgau
>> <csalgau@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>
>>> The BPF filter program currently included in vblade requires that the
>>> major and minor fields in a packet header either
>>> - match server's major and minor addresses or
>>> - be both all ones (0xffff and 0xff respectively)
>>> This is against the AoE specification that requires that the two fields
>>> be tested separately. (as seen in aoe.c:368)
>>> Proposed patch corrects this.
>>>
>>> diff --git a/vblade/bpf.c b/vblade/bpf.c
>>> --- a/vblade/bpf.c
>>> +++ b/vblade/bpf.c
>>> @@ -82,32 +82,27 @@
>>> {
>>> struct bpf_program *bpf_program;
>>> struct bpf_insn insns[] = {
>>> - /* Load the type into register */
>>> + /* CHECKTYPE: Load the type into register */
>>> BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 12),
>>> /* Does it match AoE Type (0x88a2)? No, goto INVALID */
>>> - BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0x88a2, 0, 12),
>>> + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0x88a2, 0, 10),
>>> /* Load the flags into register */
>>> BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 14),
>>> /* Check to see if the Resp flag is set */
>>> BPF_STMT(BPF_ALU+BPF_AND+BPF_K, Resp),
>>> /* Yes, goto INVALID */
>>> - BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0, 0, 9),
>>> - /* Load the shelf number into register */
>>> + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0, 0, 7),
>>> + /* CHECKSHELF: Load the shelf number into register */
>>> BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 16),
>>> - /* Does it match shelf number? No, goto CHECKBROADCAST */
>>> - BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, shelf, 0, 2),
>>> - /* Load the slot number into register */
>>> + /* Does it match shelf number? Yes, goto CHECKSLOT */
>>> + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, shelf, 1, 0),
>>> + /* Does it match broadcast? No, goto INVALID */
>>> + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xffff, 0, 4),
>>> + /* CHECKSLOT: Load the slot number into register */
>>> BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 18),
>>> /* Does it match shelf number? Yes, goto VALID */
>>> - BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, slot, 4, 0),
>>> - /* CHECKBROADCAST: is (shelf, slot) == (0xffff, 0xff)? */
>>> - /* Load the shelf number into register */
>>> - BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 16),
>>> - /* Is it 0xffff? No, goto INVALID */
>>> - BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xffff, 0, 3),
>>> - /* Load the slot number into register */
>>> - BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 18),
>>> - /* Is it 0xff? No, goto INVALID */
>>> + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, slot, 1, 0),
>>> + /* Does it match broadcast? No, goto INVALID */
>>> BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xff, 0, 1),
>>> /* VALID: return -1 (allow the packet to be read) */
>>> BPF_STMT(BPF_RET+BPF_K, -1),
>>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> "Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
>>> Instantly run your Selenium tests across 300+ browser/OS combos.
>>> Get unparalleled scalability from the best Selenium testing platform
>>> available
>>> Simple to use. Nothing to install. Get started now for free."
>>> http://p.sf.net/sfu/SauceLabs
>>> _______________________________________________
>>> Aoetools-discuss mailing list
>>> Aoetools-discuss@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.sourceforge.net/lists/listinfo/aoetools-discuss
>>
>>
>
------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
Aoetools-discuss mailing list
Aoetools-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/aoetools-discuss
