Hi Pablo, On Mon, Oct 22, 2018 at 09:45:02PM +0200, Pablo Neira Ayuso wrote: [...] > > A bit of context illustrating why I think the code needs more than just > > "more fixes": AFAIU, for each input element (which may be part of a > > range or not), code asks the kernel for whether the element exists, then > > get_set_decompose() is called to find the corresponding range. This > > approach has a critical problem though: Given a set with elements 10 and > > 20-30, asking for 10 and 20 will return the same two elements as asking > > for 10-20 does. Though in the first case, we should return 10 and 20-30 > > while in the latter case we should return nothing. > > With this patch: > > https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git/commit/?id=3b18d5eba491b2328b31efa4235724a2354af010 > > I'm going to send a pull request for David now, I guess you are > missing this kernel fix, so the _END interval flag is included when > searching for the right hand side of a matching interval. Ah! I wondered already why the test still fails although you had applied a bunch of fixes. An additional kernel fix didn't come to mind. :) > With this kernel patch plus your extended test reports I get this. > > # ./run-tests.sh testcases/sets/0034get_element_0 > I: using nft binary ./../../src/nft > > W: [FAILED] testcases/sets/0034get_element_0: expected 0 but got 1 > ERROR: asked for '22-24, 26-28', expecting '20-30' but got '20-30, 20-30' > > I: results: [OK] 0 [FAILED] 1 [TOTAL] 1 > > So, before applying your patch, I'm going to mangle it with this patch > below. If we ask for 22-24 and 26-28, the result in 20-30 and 20-30. > The command returns what is the interval container for 22-24 is 20-30, > same thing for 26-28 which is 20-30. Well, in theory, I'm fine with this. The expected outcome of 'get element' command is a matter of definition, as long as it doesn't return things which don't exist in the set. But I think the above is inconsistent with regards to single element lookups: | # nft list set ip t s | table ip t { | set s { | type inet_service | flags interval | elements = { 10, 20-30, 40, 50-60 } | } | } | # nft get element ip t s '{ 25, 28 }' | table ip t { | set s { | type inet_service | flags interval | elements = { 20-30 } | } | } Here, although I ask for two elements, a single range is returned. So I guess asking for two ranges belonging to the same range should return a single range as well. Maybe a "simple" fix would be to drop duplicates from the return set? Anyway, from my point of view your solution is fine as well: If a humanoid parser evaluates the results, it can easily spot duplicates and interpret them right. If 'get element' command is used by a script to check for existence of given ranges, it all boils down to a boolean found or not found result, so duplicates shouldn't matter that much. Thanks, Phil
