On Mon, Oct 22, 2018 at 11:14:32PM +0200, Phil Sutter wrote: > 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 } > | } > | } Using current nftables git HEAD plus kernel patch, I'm getting: # nft get element ip t s '{ 25, 28 }' table ip t { set s { type inet_service flags interval elements = { 20-30, 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. My idea was to provide list including exactly the same number of elements that have been requested, and in strict order, so you can quickly check what you request belongs to somewhere, eg. # nft get element ip t s '{ 50, 20, 10 }' table ip t { set s { type inet_service flags interval elements = { 50-60, 20-30, 10 } } } Not yet implemented, but we could add something like: # nft get element ip t s '{ 50, 20, 10 }' table ip t { set s { type inet_service flags interval elements = { 50 in 50-60, 20 in 20-30, 10 } } } So there's a clear mapping between what we request and what we get. Still, at this stage, the existing behaviour allows humans - for a small number of data - and robots, to do mappings between what they request and what they find. Thanks!
