snabb
snabb copied to clipboard
snabbco
Fix LPM4 search error on non-existent entry
When attempting to :search_bytes() for an entry which doesn't exist, an error is generated:
(1) Lua metamethod '__index' at file 'core/main.lua:168'
Local variables:
reason = string: "lib/lpm/lpm4.lua:44: attempt to index a nil value"
(*temporary) = C function: print
(2) Lua method 'search_bytes' at file 'lib/lpm/lpm4.lua:44'
This is because LPM4:search attempts to retrieve key from the returned (nil) value from :search_entry()
This fix checks that entry is not null before attempting to return the key.
The build failure appears to be just that the LPM tests need CPU affinity, or they error out. I made a patch some weeks ago to fix this but due to Snabb's needlessly complicated merge policies, now we have to live with every PR to master failing for the next month :(
/cc @lukego; #1232 fixes this error
@petebristow Added a test for nonexistent entries returning nil, and also a nonexact prefix test.
Does this suffice?
I'll merge this on the next sweep through PRs unless @petebristow has objections.
Having thought about this some more over in https://github.com/snabbco/snabb/issues/1238 and looked at my internal version. The reason the search function doesn't react well to a missing entry is because parts of the code base assume there will always be a default route. It's not documented, nor are there assertions in the code. Given that it may be a bad idea to merge this.
@petebristow so to confirm, the correct way to use the LPM library is to always add a default route entry. If there's no actual default route, storing a 'NOT FOUND' value of some sort is required?
Yes as it stands. I'm thinking it would be better if a) 0 is defined as the 'Not Found' entry b) If you don't install an explicit 0.0.0.0/0 then build adds one implicitly c) This is all documented in the README rather than a surprise feature.
What do you think ?
Yep that sounds good. This is only a problem if you don't know that LPM requires a default / not found so implicitly adding one makes sense. I suppose you could still hit this issue if you deleted the implicitly added not found but as long as it's documented i think that makes sense.