Dear Dave, On 12/9/17, Dave Barach (dbarach) <dbar...@cisco.com> wrote: > This looks wrong... vnet_set_input_acl_intfc(...) at line 93: > > /* Return ok on ADD operation if feature is already enabled */ > if (is_add && > am->classify_table_index_by_sw_if_index[ti][sw_if_index] != ~0) > return 0; > > It’s been that way for a very long time.
Yeah so I am wondering what's the right approach on fixing it, I see three alternatives: 1) "set" the new inacl even if there is an existing one applied.. upside: consistent with what "set" means in layman's terms; downside: bigger change vs. the existing semantics which maybe is masking some other issues. 2) return an error rather than zero, and let the callers deal with this. upside: no big change of the semantics. downside: returning an error might upset some callers that were "accidentally" relying on this behaviour. 3) stick in a "clib_warning()" saying "This will soon return an error. The calling code needs to ensure this is handled correctly", and wait for one or two releases, and have a JIRA for the next release to *then* do (2) in the next release. If this behavior has been here sufficiently long, (3) seems like a safest action.. What do you think ? --a > > Thanks… Dave > > From: vpp-dev-boun...@lists.fd.io [mailto:vpp-dev-boun...@lists.fd.io] On > Behalf Of Jon Loeliger > Sent: Saturday, December 9, 2017 11:23 AM > To: Andrew 👽 Yourtchenko <ayour...@gmail.com> > Cc: vpp-dev <vpp-dev@lists.fd.io> > Subject: Re: [vpp-dev] MACIP ACL replace causes ip4_table_index change? > > On Sat, Dec 9, 2017 at 8:16 AM, Andrew 👽 Yourtchenko > <ayour...@gmail.com<mailto:ayour...@gmail.com>> wrote: > Jon, > > Hi Andrew, > > Thanks for taking a look at this issue! > > on api trace: does the below work ? (even though the current scenario > is trivially reproducible, the api traces are very useful for tougher > cases, and save a lot of typing while storytelling). > > DBGvpp# api trace on > > ..... do the things .... > > DBGvpp# api trace save macip-trace > API trace saved to /tmp/macip-trace > DBGvpp# api trace custom-dump /tmp/macip-trace > SCRIPT: macip_acl_add_replace -1 count 1 count 1 \ > ipv4 permit \ > src mac 00:00:00:00:00:00 mask 00:00:00:00:00:00 \ > src ip 0.0.0.0/0<http://0.0.0.0/0>, \ > > SCRIPT: macip_acl_interface_add_del sw_if_index 0 acl_index 0 add > SCRIPT: macip_acl_add_replace 0 count 1 count 1 \ > ipv4 permit \ > src mac 00:00:00:00:00:00 mask 00:00:00:00:00:00 \ > src ip 0.0.0.0/0<http://0.0.0.0/0>, \ > > I think that is the right sequence. > > > Now, to the issue itself: it's exactly as I described, but with a twist: > vnet_set_input_acl_intfc(), which is used under the hood to assign the > inacl on the interfaces, is quite picky - if there is an existing > inacl applied, it just quietly does nothing. (@DaveB - this kinda > feels strange, I am not sure what the logic is behind doing that.) > > Anyway, rather than debating on why it behaves this way, and, > especially since we actually are deleting the tables in question, it's > better to unapply the inacls first, and then reapply them after the > tables have been recreated. > > This solves half the problem for me. It looks like I can properly > turn around and remove this ACL from the interface now! > > But I still have doubts; or at least I don't understand why the > three table indices are 3 after initial creation, and 0 after they > are replaced. > > The result is in https://gerrit.fd.io/r/#/c/9772/ - you can verify > that it addresses the issue. > > I've left a comment on the code there. > > Despite what Gerrit thinks, this code does compile and run for me! > So maybe just a "rebuild" request there will allow it to verify? > > > Now, going on to "how exactly did this slip through" - seems the macip > tests are quite a bit too lenient than they should be. I'll need to > address that as well, though probably I will split the dot1q/dot1ad > test cases out, and in the process refactor things a bit... so in the > interests of your time, maybe 9772 can go with just an actual code > fix. > > I've not read through the test as they stand today. > I'd like to understand the "3 vs. 0" issue before I am happy to > Code Review +1 this patch. > > I've dumped the entire debugging process into a log, which turned out > be fairly long, so to avoid sending the walls of text to the list, > I've dumped it elsewhere: > http://stdio.be/blog/2017-12-09-Debugging-VPP-MACIP-ACLs/ > > And, excellent! I read through that in quite some detail. And I > understand > the "3 vs 0" issue I was seeing now! > > The two pieces I missed were: The "show inacl type l2" to see where > the chain was starting, and then noticing that the chain had indeed been > reversed once the starting point was known. > > So thanks for that excellent debug layout and explanation. > > And thanks for including the intermediate step of showing why simply > updating the interface at the end wasn't sufficient. I had done that, > but hadn't yet gotten into the next function to see it was being ignored. > > Thanks again for catching this. > > Thanks for fixing this! > > > --a > > So, with that, I've left a comment and a Review -1 on the patch. > And the patch didn't Verify. So where do we go from here? > > I'm good with the patch, and we need to rebuild it. So do we just > re-build the same patch or re-submit it as a new patch? I will > either update or Review a-new to +1 it! > > Thanks, > jdl > > _______________________________________________ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
