Do a mDNS advertisement on minmdns server startup #4884

andy31415 · 2021-02-16T19:51:03Z

Problem

Devices should advertise that they exist at boot time, so that passive listeners can see such devices.

This is especially useful on commisioning devices (new devices appear) as well as in case IP address changes.

Summary of Changes

IPv4 listen on all inteerfaces (instead of NULL interface id)
at ::Start, do a mdns advertise of all PTR entries (which will recursively send SRV and A/AAAA records)

Note that this is not fully RFC compliant (RFC requires a re-advertisement after 10s). It just makes things more functional.

Fixes #4853

…plies to queries can be sent with link local information as well

…interface/capability yet

…ware of

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp

…ers (even if unlikely)

andy31415 · 2021-02-18T17:40:37Z

@woody-apple @mspang @jelderton @saurabhst @hawk248 ?

woody-apple

Looks great, thank you!

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp

github-actions · 2021-02-18T23:30:50Z

Size increase report for "nrfconnect-example-build" from 0e9c5b8

File	Section	File	VM
chip-lighting.elf	text	476	476
chip-lighting.elf	bss	0	51
chip-lighting.elf	shell_root_cmds_sections	4	4
chip-lighting.elf	[LOAD #3 [RW]]	0	-19
chip-lighting.elf	rodata	-32	-32
chip-lock.elf	text	472	472
chip-lock.elf	bss	0	55
chip-lock.elf	shell_root_cmds_sections	8	8
chip-lock.elf	[LOAD #3 [RW]]	0	-23
chip-lock.elf	rodata	-32	-36

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,4206
.debug_str,0,1147
.debug_line,0,648
text,476,476
.debug_loc,0,441
.debug_ranges,0,440
.strtab,0,315
.symtab,0,192
.debug_frame,0,76
bss,51,0
.debug_aranges,0,16
.debug_abbrev,0,10
shell_root_cmds_sections,4,4
.shstrtab,0,-3
[LOAD #3 [RW]],-19,0
rodata,-32,-32

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,20
.debug_str,0,17
.debug_loc,0,-5

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,4206
.debug_str,0,1147
.debug_line,0,652
text,472,472
.debug_loc,0,445
.debug_ranges,0,440
.strtab,0,315
.symtab,0,192
.debug_frame,0,76
bss,55,0
.debug_aranges,0,16
.debug_abbrev,0,10
shell_root_cmds_sections,8,8
.shstrtab,0,-3
[LOAD #3 [RW]],-23,0
rodata,-36,-32

github-actions · 2021-02-18T23:31:06Z

Size increase report for "gn_qpg6100-example-build" from 0e9c5b8

File	Section	File	VM
chip-qpg6100-lock-example.out	.text	476	476
chip-qpg6100-lock-example.out	.bss	0	40
chip-qpg6100-lock-example.out	.heap	0	-40

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lock-example.out and ./pull_artifact/chip-qpg6100-lock-example.out:

sections,vmsize,filesize
.debug_info,0,4372
.debug_str,0,1153
.debug_loc,0,877
.debug_line,0,871
.strtab,0,573
.text,476,476
.symtab,0,320
.debug_frame,0,156
.debug_ranges,0,88
.bss,40,0
.debug_aranges,0,40
.shstrtab,0,-1
.heap,-40,0
.debug_abbrev,0,-57
[Unmapped],0,-476

github-actions · 2021-02-18T23:45:59Z

Size increase report for "esp32-example-build" from 0e9c5b8

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	504	504
chip-all-clusters-app.elf	.dram0.bss	0	40
chip-all-clusters-app.elf	.flash.rodata	-32	-32

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize
.debug_str,0,663
.debug_info,0,374
.debug_loc,0,2
.xt.prop._ZSt9__find_ifIPKSt4byteN9__gnu_cxx5__ops10_Iter_predIPFbS0_EEEET_S9_S9_T0_St26random_access_iterator_tag,0,1

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,4185
.debug_line,0,1345
.debug_str,0,1147
.debug_loc,0,648
.debug_ranges,0,592
.flash.text,504,504
.strtab,0,139
.dram0.bss,40,0
.symtab,0,32
[Unmapped],0,32
.debug_frame,0,24
.debug_aranges,0,8
.xt.prop._ZN4mdns7Minimal16QueryReplyFilter6AcceptENS0_5QTypeENS0_6QClassENS0_9FullQNameE,0,4
.shstrtab,0,1
.debug_abbrev,0,-9
.flash.rodata,-32,-32
.xt.prop._ZN4mdns7Minimal16QueryReplyFilterD2Ev,0,-40

src/lib/mdns/minimal/Server.h

src/lib/mdns/minimal/responders/QueryResponder.h

mspang · 2021-02-19T17:48:35Z

src/lib/mdns/minimal/QueryReplyFilter.h

 private:
+    bool AcceptableQueryType(QType qType)
+    {
+        if (mSendingAdditionalItems && mQueryData.IsBootAdvertising())


Is this check needed (isn't the 2nd condition true already?)

IsBootAdvertising is only set during initial boot, not during normal query replies, where as 'sending additional items' is set for any reply processing. I believe they are both needed.

mspang · 2021-02-19T17:49:29Z

src/lib/mdns/minimal/QueryReplyFilter.h

+
+    bool AcceptablePath(FullQName qname)
+    {
+        if (mIgnoreNameMatch || mQueryData.IsBootAdvertising())


Why do we need IsBootAdvertising instead of just using IgnoreNameMatch ?

It seemed to make the intent cleaner by having behaviour depend on query.

The boot-time broadcaster builds a fake query (so that the sender sends replies to such a query) however it does not have access to the underlying filtering used by the mDNS sender.

Alternative would be to propagate flags into the mdns sending and that seeemed to be a bit uglier.

mspang · 2021-02-19T21:42:56Z

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp

+        return false;
+    }
+
+    if (strncmp(name, "lo", 2) == 0)


Not a new issue, but doesn't seem right that if I rename my interface to "lollipop" it will be skipped.

Probably the name is the wrong thing to look at altogether, and we should just inspect the address.

I could find no better way.

For devices this may not be an issue (since they control their wifi name and it is likely LWIP anyway). Would people rename interfaces on android/iOS?

Added a TODO to auto-create an issue.

Can't you just get the address and see if it's ::1 or inside 127.0.0.0/8 ?

InterfaceIterator does not provide the address only InterfaceAddressIterator does.

Absolutely possible to use AddressIterator instead and do some dedup, but I find it more bothersome than what I would be generally add as a 'fix if you are already touching this'.

Real fix would probably to fix the interface iterator to be able to return an address iterator for that specified interface. Having 2 types of iterators that both iterate over all interfaces is strange.

todo · 2021-02-19T21:51:10Z

need a better way to ignore local loopback interfaces/addresses

connectedhomeip/src/lib/mdns/Advertiser_ImplMinimalMdns.cpp

Lines 105 to 115 in d285703

    
           // TODO: need a better way to ignore local loopback interfaces/addresses 
        
           // We do not want to listen on local loopback even though they are up and 
        
           // support multicast 
        
           // 
        
           // Some way to detect 'is local looback' that is smarter (e.g. at least 
        
           // strict string compare on linux instead of substring) would be better. 
        
           // 
        
           // This would reject likely valid interfaces like 'lollipop' or 'lostinspace' 
        
           if (strncmp(name, "lo", 2) == 0) 
        
           { 
        
               /// local loopback interface is not usable by MDNS

This comment was generated by todo based on a `TODO` comment in `d285703` in #4884. cc @andy31415.

…. Better to log instead of dying

andy31415 added 3 commits February 16, 2021 10:25

Listen on all IPv4 interfaces (not only NULL interface ID) so that re…

e08ed9b

…plies to queries can be sent with link local information as well

Do a startup mDNS broadcast for available services - no filtering by …

67cd75b

…interface/capability yet

Ensure mdns advertise at boot only happens on interfaces that it is a…

8d72191

…ware of

boring-cyborg bot added the lib label Feb 16, 2021

pullapprove bot requested review from bzbarsky-apple, chrisdecenzo, hawk248, jelderton, msandstedt, mspang, saurabhst and woody-apple February 16, 2021 19:53

andy31415 added 2 commits February 16, 2021 14:56

Remove FIXME since it was already addressed

df6955d

Revise doc comment on boot time advertisement

86acb92

msandstedt reviewed Feb 16, 2021

View reviewed changes

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp Outdated Show resolved Hide resolved

Add a constant for maximum interface name length

e28ffc6

boring-cyborg bot added examples inet transport labels Feb 17, 2021

restyled-io bot mentioned this pull request Feb 17, 2021

Restyle Do a mDNS advertisement on minmdns server startup #4898

Closed

Restyle fixes

c047193

pullapprove bot requested a review from rwalker-apple February 17, 2021 14:07

andy31415 added 2 commits February 17, 2021 09:08

Fix LWIP interface name max size

d4b0efc

Increase LWIP max size some more, to support for 32bit interface numb…

08d420e

…ers (even if unlikely)

andy31415 requested a review from msandstedt February 17, 2021 14:11

andy31415 added 2 commits February 17, 2021 14:30

Update include file for zephyr interface name constant

756d32a

Merge branch 'master' into 01_mdns_advertise_at_boot

07e3e79

msandstedt approved these changes Feb 17, 2021

View reviewed changes

woody-apple approved these changes Feb 18, 2021

View reviewed changes

bzbarsky-apple approved these changes Feb 18, 2021

View reviewed changes

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp Outdated Show resolved Hide resolved

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp Outdated Show resolved Hide resolved

src/lib/mdns/Advertiser_ImplMinimalMdns.cpp Outdated Show resolved Hide resolved

Deduplicate the detection of usable mdns interfaces

9fecbf9

andy31415 force-pushed the 01_mdns_advertise_at_boot branch from f3fd002 to 9fecbf9 Compare February 18, 2021 23:00

andy31415 added 3 commits February 18, 2021 18:04

Deduplicate ip addresses for global mdns broadcast

e402b1d

Fix compilation

2157ffc

Fix linkage too

c6127f6

mspang reviewed Feb 19, 2021

View reviewed changes

andy31415 added 3 commits February 19, 2021 16:39

do not use pre-main initialization for mDNS broadcast IP addresses

8c91e4d

Fix typo: Trottle -> Throttle

708772e

Fix typo: Trottle -> Throttle

076431b

mspang approved these changes Feb 19, 2021

View reviewed changes

Add todo to have better local loopback detection for interfaces

d285703

andy31415 added 3 commits February 19, 2021 17:04

Log on IP parse error - we seem to crash in esp32 and am not sure why…

68527cf

…. Better to log instead of dying

Fix condition inversion

fba34ab

Fix logic error: IP address returns a bool not CHIP_ERROR

88d1444

andy31415 merged commit 2b58bdf into project-chip:master Feb 19, 2021

todo bot mentioned this pull request Feb 19, 2021

need a better way to ignore local loopback interfaces/addresses #4950

Closed

andy31415 deleted the 01_mdns_advertise_at_boot branch October 28, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do a mDNS advertisement on minmdns server startup #4884

Do a mDNS advertisement on minmdns server startup #4884

andy31415 commented Feb 16, 2021 •

edited

Loading

andy31415 commented Feb 18, 2021

woody-apple left a comment

github-actions bot commented Feb 18, 2021

github-actions bot commented Feb 18, 2021

github-actions bot commented Feb 18, 2021

mspang Feb 19, 2021

andy31415 Feb 19, 2021

mspang Feb 19, 2021

andy31415 Feb 19, 2021

mspang Feb 19, 2021

andy31415 Feb 19, 2021

mspang Feb 19, 2021

andy31415 Feb 19, 2021

andy31415 Feb 19, 2021

todo bot commented Feb 19, 2021

Do a mDNS advertisement on minmdns server startup #4884

Do a mDNS advertisement on minmdns server startup #4884

Conversation

andy31415 commented Feb 16, 2021 • edited Loading

Problem

Summary of Changes

andy31415 commented Feb 18, 2021

woody-apple left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2021

github-actions bot commented Feb 18, 2021

github-actions bot commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todo bot commented Feb 19, 2021

need a better way to ignore local loopback interfaces/addresses

This comment was generated by todo based on a TODO comment in d285703 in #4884. cc @andy31415.

andy31415 commented Feb 16, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `d285703` in #4884. cc @andy31415.