lib: make map_contains recongize null value

Without changing the interface, map_contains is able to tell whether the item exist in map or not.

More about the intension: module stats use map internally, metric with zero value is not being able to retrieve by stats_get since map_contains checks the return value against NULL.

@pspacek is there anything I could do to move this forward?

@anb we're being over-busy ATM.

Please make sure all tests pass. If it was in infrastructure issue then please retry the test. Also, it will help if you provide more details about "make some tests success for stats module" you mention in MR description.

Maybe just add the tests you have in mind into separate commit of this MR so we can have a look what is the intention.

changed the description

Hmm, this is weird, stats module seems to work for me even with 0 values:

[system] interactive mode
> modules = { 'stats' }

> stats.list()
[answer.nxdomain] => 0
[answer.100ms] => 0
[answer.1500ms] => 0
[answer.slow] => 0
[answer.servfail] => 0
[answer.250ms] => 0
[answer.cached] => 0
[answer.nodata] => 0
[query.edns] => 0
[query.dnssec] => 0
[answer.total] => 0
[answer.10ms] => 0
[answer.noerror] => 0
[answer.50ms] => 0
[answer.500ms] => 0
[answer.1000ms] => 0
[answer.1ms] => 0

> stats.get('answer.1ms')
0

added needinfo label

I think that's because answer.* is predefined constant metrics, which are not stored in map: https://gitlab.labs.nic.cz/knot/knot-resolver/blob/master/modules/stats/stats.c#L278-284

> stats['my.test'] = 0

> stats.list()
[answer.nxdomain] => 0
[answer.100ms] => 0
[answer.1500ms] => 0
[answer.slow] => 0
[answer.servfail] => 0
[answer.250ms] => 0
[answer.cached] => 0
[answer.nodata] => 0
[my.test] => 0
[dnslog.sent.total] => 1
[query.edns] => 1
[query.dnssec] => 0
[answer.total] => 1
[answer.10ms] => 1
[answer.noerror] => 1
[answer.50ms] => 0
[answer.500ms] => 0
[answer.1000ms] => 0
[answer.1ms] => 0

> stats.get('my.test')
nil

> stats.get('answer.slow')
0

P.S. seems I cannot make the coverage pipeline work by simply restarting it. :(

Please ignore the error in coverage target in pipeline, it is some glitch in Gitlab.

Besides this, I wonder if it is appropriate to modify map library instead of fixing this in stats module.

In any case, please provide test for this behavior. You can take some inspiration from git/modules/policy/policy_test.lua. To run these tests use make installcheck.

Apologize, I didn't make it clear in the first place. The change is for map library, and it already has a test for null value entry.

assigned to @vcunat

mentioned in merge request !456 (merged)

Ah, the stats module casts the integers into void *, and that's why the NULLs. Lovely.

Rebased to avoid adding two merge commits.

closed

Thank you!

Well, I was getting to question if we should fix stats module instead ...

added bug and removed needinfo labels

It was a semantic bug in map implementation anyway. If you inserted a NULL, it would be stored but always appear as if not stored. That was just strange.

Thank you for fixing the comments. :)

Ok, fine with me.