mDNS & DNS-SD

[jelledevleeschouwer]

Pull request for updated mDNS module and new DNS-SD module.

[danielinux]

Unit tests failing when merging to the latest dev branch.

Details below:

86%: Checks: 65, Failures: 0, Errors: 9
test/unit/modunit_pico_mdns.c:836:E:Unit test for mdns_cookie_resolve_conflict:tc_mdns_cookie_resolve_conflict:0: (after this point) Received signal 6 (Aborted)
test/unit/modunit_pico_mdns.c:1734:E:Unit test for mdns_my_records_add:tc_mdns_my_records_add:0: (after this point) Received signal 11 (Segmentation fault)
test/unit/modunit_pico_mdns.c:1999:E:Unit test for mdns_cache_add_record:tc_mdns_cache_add_record:0: (after this point) Received signal 11 (Segmentation fault)
test/unit/modunit_pico_mdns.c:2025:E:Unit test for mdns_cache_flush:tc_mdns_cache_flush:0: (after this point) Received signal 11 (Segmentation fault)
test/unit/modunit_pico_mdns.c:2256:E:Unit test for mdns_apply_known_answer_suppression:tc_mdns_apply_known_answer_suppression:0: (after this point) Received signal 6 (Aborted)
test/unit/modunit_pico_mdns.c:2298:E:Unit test for mdns_getrecord:tc_mdns_getrecord:0: (after this point) Received signal 11 (Segmentation fault)
test/unit/modunit_pico_mdns.c:1374:E:Unit test for mdns_announce:tc_mdns_announce:0: (after this point) Received signal 11 (Segmentation fault)
test/unit/modunit_pico_mdns.c:1374:E:Unit test for mdns_probe:tc_mdns_probe:0: (after this point) Received signal 11 (Segmentation fault)
test/unit/modunit_pico_mdns.c:2451:E:Unit test for mnds_claim:tc_mdns_claim:0: (after this point) Received signal 6 (Aborted)

[jelledevleeschouwer]

Hmm, thats odd. I have no errors locally not even when I checkout the dns-sd-dev branch somewhere else and build it again. I use following command:

make clean units ARCH=faulty

Could this be a platform issue? Maybe then I could reproduce the errors?

[danielinux]

I am getting the same before the merge.

My platform: debian-sid, 32bit, Linux 3.16.0-4-686-pae #1 SMP Debian 3.16.7-ckt2-1

my command:

make clean
make units ARCH=faulty

[jelledevleeschouwer]

I'll try to reproduce & find the causes.

[jelledevleeschouwer]

Found the causes and fixed the errrors.

Most of the errors where caused by wrong DNS name conversion-functions in the DNS Common module. I had already updated those functions in some other directory earlier on, but forgot to update them in the actual developing directory.
Another error was caused by freeing memory of the received buffer.

Strange the unit tests didn't fail on Ubuntu though. Normally, all unit tests should run smooth now.

[coveralls]

Changes Unknown when pulling 1b338a6 on dns-sd-dev into * on development*.

[danielinux]

Thanks @jelledevleeschouwer - All my tests are passing now. Merge should happen soon, stay tuned.

[danielinux]

Hello @jelledevleeschouwer - we just fixed the format of pico_dns_name_to_dns_notation() and pico_dns_notation_to_dns_name() in dns_common,
(see 4d3522c)
could you please adjust your branch accordingly?

It is not safe to call these function without any boundary, so we added a maximum length as a second argument passed to these two functions. The old mdns module has been temporarily reworked according to this, but it would be much nicer if you could review all the function calls in the new mdns module towards pico_dns_name_to_dns_notation and pico_dns_notation_to_dns_name, especially that the correct length is passed upon every call.

Thanks, and sorry about this. The branch is still under review, and should be merged soon.

[jelledevleeschouwer]

No problem, makes sense, I'll fix it somewhere this weekend!
(Together with some other small updates to improve complexity and safety)

[jelledevleeschouwer]

I might have to push some more changes today/tomorrow to add support for NSEC records. NSEC records is the only point my implementation differs from the real Bonjour implementation. So I'll add it today.

I'm sorry to slow the pull-request process down.

[danielinux]

Hey @jelledevleeschouwer great job so far, we also enjoyed your video on youtube.
I have another small request to make: could you please update docs/user_manual/chap_api_mdns adding a section about dns_sd_* functions usage?

Thanks !!

[jelledevleeschouwer]

Will do 👍

[jelledevleeschouwer]

Hey @danielinux,

Overall quality should be better now and all memory structures are updated to use 'pico_tree' instead. Documentation follows on thursday, first going to work on my presentation a bit.

[danielinux]

Hi Jelle,

Thank you, the code has improved a lot.

• Please can you double check my fix in 8181c39 ?
• Yes, API documentation is a must have for inclusion
• please can you git merge from development branch? I see your branch base is outdated, and my attempt of automatic merge fail
• Can you please explain what's behind the > 20% Unit test coverage drop by the way? Is there any test that has been discarded? There seems to be a problem with other modules as well, because the coverage has dropped in TICS.

Thanks

[jelledevleeschouwer]

Hi @danielinux,

First of all, I've written API documentation for the mDNS-module as well as the DNS SD-module.
Secondly, I've merged the development-branch after a few tries. Need to improve my git-skills though 😄

About the TICS coverage, I assume you mean the drop around May 4th for mDNS-module? To be honest I have no clue. Because since the first run through TICS somewhere in april, I've actually even added unit tests, so I would assume the coverage increases because of that. Does it need improvement?

Thank you,

Jelle

[danielinux]

Hi Jelle, sorry to see that the merge was so painful. When I maintain a branch, I do my left-to-right merges every day to avoid the fork to be too wide and end up in that situation.

The regressions I was seeing on TICS coverage was not related to the module itself, but spread across other modules. I am now re-running the showcase, I'll comment on the results soon.

[danielinux]

@jelledevleeschouwer it looks like your merge was not successful, as you discarded a lot of fixes from the development branch. Would you attempt that again?

Thanks

-d

[jelledevleeschouwer]

Hi @danielinux,

Thank you, that indeed was the case, I think I finally figured out how to fix all my problems. Here's how:

I checked out the development branch locally and merged my feature-branch into it, as if it would when this pull-request would pass. Then I updated the remote tracking branch to dns-sd-dev and just forced pushed to the remote dns-sd-dev. This destroyed all my previous commits of course, but I think this is better than fixing all those conflicts manually or losing all my work. Now the dns-sd-dev branch is actually just the development branch but with mDNS- and DNS-SD-modules.

Sorry about all the troubles, my git skills aren't the best yet. It should be solved now, though 👍

Jelle

[danielinux]

@jelledevleeschouwer please install "uncrustify" and run "make style". This will fix indentation and coding style automatically, so we won't lose your authoring when merging. Thank you!

[danielinux]

I added the public prototype

int pico_mdns_flush_cache( void );

to the pico_dns_common.h header, because it was never called within your code. If this is an API function, it needs to be documented. Otherwise it needs to go...

[jelledevleeschouwer]

@danielinux, Thanks a lot for the fixes,

I've deleted the cache-flush since it isn't really necessary and doesn't add much value. Also 'made style' with uncrustify.