Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix LPM4 search error on non-existent entry #1223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benagricola
Copy link
Contributor

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.

@wingo
Copy link
Contributor

wingo commented Sep 21, 2017

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
Copy link
Contributor

Hi @benagricola,

Would you mind adding a test for this ?

Pete

@benagricola
Copy link
Contributor Author

@petebristow Added a test for nonexistent entries returning nil, and also a nonexact prefix test.

Does this suffice?

@lukego
Copy link
Member

lukego commented Nov 17, 2017

I'll merge this on the next sweep through PRs unless @petebristow has objections.

@petebristow
Copy link
Contributor

Looks good to me.

@petebristow
Copy link
Contributor

Having thought about this some more over in #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.

@benagricola
Copy link
Contributor Author

@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?

@petebristow
Copy link
Contributor

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 ?

@benagricola
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants