patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9555, Add MDNS probing

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9555: Add MDNS probing

Submitted by:  Jens Nielsen <deft>
Submitted on:  Sun 28 Jan 2018 07:41:50 AM UTC  
 
Category:  apps Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Simon Goldschmidt <goldsimon> Open/Closed:  Closed
Planned Release:  2.1.0

( Jump to the original submission)

Tue 03 Jul 2018 06:49:15 PM UTC, comment #20:

Right. I copied the other existing places of similar code. This could be changed, but I wanted all occurrences to be equal.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 03 Jul 2018 10:16:06 AM UTC, comment #19:

Yes, sorry I meant to go through the code and remaining todos and do some tests but I think it looks good. I'll open a new patch if I find something useful to share.

The only minor comment from my side is that the two LWIP_MIN(MDNS_LABEL_MAXLEN, len) added in 4506db4331ad1426e5589701d7b193a074aabfee should be unnecessary since there is an LWIP_ERROR to return an error if len > MDNS_LABEL_MAXLEN? Or otherwise the string null termination on the line below also needs LWIP_MIN

Anyway that's not important. Thanks a lot for the help Simon!

Jens Nielsen <deft>
Tue 03 Jul 2018 08:48:56 AM UTC, comment #18:

OK, so I'll close this as done. Please reopen if this is wrong.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 28 Jun 2018 08:13:51 PM UTC, comment #17:

I've finally merged your patch and added some cleanup on top.

I don't know if it fully works (I've only done some quick tests), but even if it doesn't (what I don't expect :), it's a start in the right direction ;-)

Should I leave this open for the remaining todos?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 22 Jun 2018 08:44:09 PM UTC, comment #16:

Actually, in this case I find it easier to review the squashed version to see what's changed why. I'll still need some time to review but overall it looks good I think.

Assigning this to me as noone else seems to take over :-/

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 22 Jun 2018 09:59:40 AM UTC, comment #15:

Attaching MDNS-probing-squashed.patch which contains a rebased and squashed version of all the other patches. "mdns probe test.pcap" demonstrates some of my tests, first 192.168.5.26 starts and registers its names and then I try to start up 192.168.5.62 with different name conflicts (and once without conflict in the end). The responses from .26 indicating conflict are sent unicast so they can't be seen here since the capture is taken on my pc, but you can see my "conflict resolver" function adding the "(2)" to the name and restarting a new series of probes.

Also attaching the old missing 0006 patch in case anyone would be interested

(file #44413, file #44414, file #44415)

Jens Nielsen <deft>
Fri 22 Jun 2018 06:28:51 AM UTC, comment #14:

Sure, I also see the patches doesn't apply on latest master anymore, I will squash and rebase and get back with a new patch once I've done some tests.

In any case, all of them are applicable in numerical order except for 0003 which has a version 2 and for some reason I messed up attaching 0006. It made sense to split in the beginning because you might want to keep some parts and do some differently and also it's quite big so maybe easier to review in steps, but I agree it got confusing

Jens Nielsen <deft>
Thu 14 Jun 2018 11:07:36 AM UTC, comment #13:

Reading through this patch again, I'm confused. I even don't know which one of the attached files to check.

Would you mind uploading again what you think is a clean patch that is tested and applies? Together with a description of what the patch does/changes? That would greatly ease up my task of reviewing this.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 13 Jun 2018 05:47:35 PM UTC, comment #12:

Yes I noticed 2.1.0 is getting closer, hence the question ;) If you have time to look at it that would be super, unfortunately I will be out of office until mid next week but if you need anything else from me I'd be happy to help

Jens Nielsen <deft>
Tue 12 Jun 2018 08:29:12 PM UTC, comment #11:

Well, you might have noticed leftovers for 2.1.0 are decreasing. We are (hopefully) talking about days not weeks here. We really need a decision here, anyway, so I hope I can find the time to have a look at this the next days ;-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 12 Jun 2018 02:39:45 PM UTC, comment #10:

Hi guys I know you're all busy and I don't want to nag, but for my own project planning purposes would there be any chance of getting this in the next release?

To recap why I think this is important, the current mdns responder is nice but not compliant, as is also mentioned in the documentation. Without this the user must make sure the name is unique on the network, which may be ok for some products but for others not really acceptable.

It would also need tiebreaking to be compliant in this aspect but this at least takes it one step closer. (obviously I'm biased to believe my code is completely correct ;) )

Jens Nielsen <deft>
Sun 18 Feb 2018 01:48:59 PM UTC, comment #9:

Oops I realize I forgot to upload the two rename functions I promised in comment 6, adding patch 6 with the following addition to the api:

This allows an application to resolve a name conflict (and change a name at runtime, in case the application provides an interface to the user to set a new arbitrary name)

Jens Nielsen <deft>
Thu 08 Feb 2018 11:38:13 AM UTC, comment #8:

I've reverted the accidental commit d6e58d02a619813a4187ab53ab0ffc2f0b864ad7 for now.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 06 Feb 2018 08:09:30 PM UTC, comment #7:

Erik, it seems you accidentally pushed one patch, should we revert it?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 06 Feb 2018 07:31:39 PM UTC, comment #6:

Hi Axel,

Yes you're right, I'm already losing track of my own patches but mdns_resp_start() shouldn't be necessary if all this gets approved. Before this commit you were supposed to call mdns_resp_announce() when all services had been added but this should also not be necessary. I have updated some of the documentation in patch 5 but this needs to be revisited again, I see UPGRADING mentions mdns_resp_announce()

I have also prepared two new api functions to allow renaming netif hostname or service name so user won't have to call mdns_resp_remove_netif() and add everything again (which would trigger igmp leave/join). I will test it within a couple of days and upload another patch...

With this the only thing I'm not happy with in the api is mdns_name_result_cb_t which doesn't tell the user whether a conflict was on the netif or a service, or which service. I don't need to know in my application but someone might...

Jens Nielsen <deft>
Tue 06 Feb 2018 03:00:42 AM UTC, comment #5:

Hi,
Since commit d6e58d02a619 ("MDNS send probes to verify domain before use"),
now I need to explictly adding mdns_resp_start() call to make my application work.
Maybe need to update doc/mdns.txt and UPGRADING files, so existing users won't be surprised.

BTW, it looks like mdns_resp_start() will be removed by below patch, so maybe don't need to update the document after below patch.
[PATCH 4/4] Remove superfluous mdns_resp_start api

Axel Lin <axellin>
Project Member
Fri 02 Feb 2018 10:22:49 AM UTC, comment #4:

Oops, it seems I pushed the first patch just now when I was fixing another thing.

Will look into your updated patches.

Erik Ekman <yarrick>
Project Member
Mon 29 Jan 2018 08:04:33 AM UTC, comment #3:

Ah right, I think the NULL dereference is copypasted from mdns_announce, I'll fix both

Regarding the authoritative answers you're probably right, the RFC says this in section 8.2:

"each host populates the query message's Authority Section with the record or records with the rdata that it would be proposing to use, should its probing be successful."

and

"for tiebreaking to work correctly in all cases, the Authority Section must contain all the records and proposed rdata being probed for uniqueness."

I just went with all, but the authority section should just contain a proposed answer to the query. If someone else probes for the same name as us we'll check the other node's authority section for answers for that name and compare rdata to see which one should defer to the other. Unless I greatly misunderstood something there's no point in including any of the PTR records, not the two you mentioned or the in-addr.arpa one either, since we're not querying for that.

I also rebooted that other computer I have on my network to see what it sent and it only had an A, AAAA and SRV record in the probes, while its announce message looks like ours.

Adding a new version of patch 3 to fix since this reverts a large part of the original version.

Also adding patch 5 to fix the NULL dereference fix and documentation updates. I haven't documented the name resolution api properly yet since I guess it's not settled yet. I was thinking I could add mdns_resp_rename_netif() and _rename_service() so the user doesn't have to call mdns_resp_remove_netif() and add everything again?

(file #43103, file #43104, file #43105)

Jens Nielsen <deft>
Sun 28 Jan 2018 08:36:23 PM UTC, comment #2:

Thanks, this looks very promising!

I don't think the nullcheck works in mdns_resp_restart() since NETIF_TO_HOST already deferences it:
+ struct mdns_host* mdns = NETIF_TO_HOST(netif);
+ LWIP_ASSERT_CORE_LOCKED();
+ LWIP_ERROR("mdns_resp_start: netif != NULL", (netif != NULL), return);

Not sure if it makes sense to include the _services._dns-sd._udp.local -> service type PTR and also the service type to service name PTR among the probe authorative answers. Does the RFC give guidance on this? Or other implementations?

Erik Ekman <yarrick>
Project Member
Sun 28 Jan 2018 08:30:10 PM UTC, comment #1:

An idea regarding the api, the user saying "all services are added, go ahead" call isn't really needed, that was just to prevent a stream of announce messages at startup, right? Now I can just (re-)start the timer when the netif or a service is added and assuming all services are added before the timer expires we get the same result. Otherwise the worst thing that will happen is that we send some unnecessary probes but in the end we have still probed all records for uniqueness, with a bit less confusing api.

Proposal for this change (apply on top of patch 1) in patch 4

(file #43100)

Jens Nielsen <deft>
Sun 28 Jan 2018 07:41:50 AM UTC, original submission:

Hi

I had a go at MDNS probing, as described in RFC 6762 Section 8.1. The implementation I have here basically just covers my needs and is barely tested and most likely contains a few bugs, but I add it here as a request for comments to see if we can agree on a path to get it into lwip

Patch 1 contains the basics, send 3 probe queries before responding to any queries, and abort if someone answers. I know this breaks backwards compatibility in the api by adding the _start() and _restart() functions but I don't know how else I can differentiate the three distinct events: 1. User saying "all services are added, go ahead", 2. Cable has been reconnected or administrative interface came up after being down and probing must be restarted, and 3. The IP address has changed, send a new announce message but don't restart probing.
I'm also aware this doesn't comply with some other MUST requirements (such as the 5 second delay or using the 5353 source port for queries if that applies here) but the whole probing section is a MUST so, one step at a time...

Patch 2 contains a minimal set of what I need to handle conflict, a simple callback with a result. If result is 0 I call mdns_resp_remove_netif() and try again with a new name, if result is 1 I can write the name to nonvolatile storage. This works for me since I will use same name for host and services but probably needs to be extended to cover the case where someone has different names, though I don't really understand the use case for that in an embedded device... I couldn't come up with a decent api to cover that so I left it out but should probably be considered if merging.

Patch 3 contains an approach to populate the authoritative section of the probe request, according to Section 8.2. Strictly not needed for probing but there was a comment somewhere in the RFC that this could be used to differentiate a probe from a normal query.

Also attached a wireshark trace to show what it looks like. 192.168.5.44 is some other computer on my network, 192.168.5.26 is my lwip device that I had configured to use the same host name as that computer.

Jens Nielsen <deft>

 

Attached Files
file #44414:  mdns probe test.pcap added by deft (11KiB - application/octet-stream)
file #44415:  MDNS-probing-squashed.patch added by deft (23KiB - application/octet-stream)
file #43104:  mdns probe v2.pcap added by deft (1KiB - application/octet-stream)
file #43105:  0005-MDNS-minor-fixes-and-documentation.patch added by deft (5KiB - application/octet-stream)
file #43099:  mdns probe.pcap added by deft (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by axellin (Posted a comment)
  • -email is unavailable- added by yarrick (Posted a comment)
  • -email is unavailable- added by deft (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-07-03 goldsimon StatusIn Progress => Done
        Open/ClosedOpen => Closed
    2018-06-22 goldsimon StatusNone => In Progress
        Assigned toNone => goldsimon
        Planned ReleaseNone => 2.1.0
    2018-06-22 deft Attached File- => Added 0006-MDNS-add-rename-functions-for-netif-and-services.patch, #44413
        Attached File- => Added mdns probe test.pcap, #44414
        Attached File- => Added MDNS-probing-squashed.patch, #44415
    2018-01-29 deft Attached File- => Added 0003-Populate-MDNS-probes-authoritative-section_v2.patch, #43103
        Attached File- => Added mdns probe v2.pcap, #43104
        Attached File- => Added 0005-MDNS-minor-fixes-and-documentation.patch, #43105
    2018-01-28 deft Attached File- => Added 0004-Remove-superfluous-mdns_resp_start-api.patch, #43100
    2018-01-28 deft Attached File- => Added 0001-MDNS-send-probes-to-verify-domain-before-use.patch, #43096
        Attached File- => Added 0002-Add-callback-to-user-to-notify-result-of-mdns-probe.patch, #43097
        Attached File- => Added 0003-Populate-MDNS-probes-authoritative-section.patch, #43098
        Attached File- => Added mdns probe.pcap, #43099

    Back to the top


    Powered by Savane 3.4