patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9339, mdns: support for deleting...

 
 

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

patch #9339: mdns: support for deleting responder services and searching services on network

Submitted by:  David GIRAULT <dgirault>
Submitted on:  Wed 10 May 2017 08:26:00 AM UTC  
 
Category:  apps Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

( Jump to the original submission)

Mon 07 Aug 2017 10:16:17 AM UTC, comment #15: 

Closed as requested by David. (-> new patch for mdns search part)

Dirk Ziegelmeier <dziegel>
Project Administrator
Fri 04 Aug 2017 07:37:28 AM UTC, comment #14: 

Since this one was partially merged (the responder service part) and I've made some change in the searching part, I'll make a new patch which will address only the mdns search part.
So I think this one can be closed.
Regards,
David

David GIRAULT <dgirault>
Wed 31 May 2017 11:42:19 AM UTC, comment #13: 

Hi Simon,

I had changed the stop function to take a u8 request_id parameter. Callback isn't called anymore during start and request_id is returned in additional pointer parameter.

See new attachement.

mdns.c search service don't support other type of request nor caching. So unicast request and caching through dns.c is still usefull. But with more work on mdns search, it would make sense to have dns.c relying on mdns.c for .local lookup.

globally, a common DNS caching api, which will hold all kind of DNS/mDNS answer type (A,AAAA,PTR,etc.) will allow the two services to interact smoothly on a common database and permit to store known services. But, this don't avoid sending new request if previous request don't return expected results.

(file #40822)

David GIRAULT <dgirault>
Wed 17 May 2017 06:55:00 PM UTC, comment #12: 

> have you a .astylerc configuration I can use?


Unfortunately not. Mighte make sense though...

As to the stop function: I would prefer not to call the result_fn callback during start but to let 'mdns_search_service' return the 'mdns_request' pointer.

Regarding the dns.c/mdns.c questions:
- do you think it would make sense to let DNS use the new mdns.c functions to search for names?
- do we need a table for known services to prevent looking them up again too fast? I think the mdns spec said something about that... And I wouldn't want to implement searching that doesn't comply to the spec. I'm too lazy to read up on the spec right now though...

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 12 May 2017 02:05:10 PM UTC, comment #11: 

Nice to have patch #1 already pushed.

Now for the 2nd patch:

- for coding style, what kind of style error? have you a .astylerc configuration I can use?

- the stop function should be called from the result_fn callback, where the mdns_request pointer is known.
the result_fn callback is called at least once. the first call is synchronous during the call to `mdns_search_service` before it return and allow application to store the `mdns_request` pointer. this allow application to call stop fnuction later if no answer were received (timeout handling done by app).

- mdns code in dns.c don't allow browsing the network for services like this patch does. it only allow doing an A or AAAA request. as you can see, mdns_search_service() does a PTR request. additionnaly, all answer RR included in all response received are sent to application provided callback.
The last answer RR in a received frame is flagged to allow application to detect multiple responses.

- request sent by dns.c are flagged so response are unicast. mdns request sent by my patch result in multicast response, so received by mdns multicast udp socket itself.

- mdns.c already provided most of decoding of dns frame and has support for PTR, SRV, and TXT RR.

- dns_gethostbyname() may be still used to search for '.local' domain.

David GIRAULT <dgirault>
Fri 12 May 2017 01:21:33 PM UTC, comment #10: 

OK, to the 2nd patch:
- coding style is not fully conformant
- I don't get how the stop function works (where would an application get the 'mdns_request' pointer from to stop it?)
- We already have code sending mdns requests in dns.c for link-local name resolution. How does your change relate/integrate with that?

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 12 May 2017 11:23:05 AM UTC, comment #9: 

Then let's do it like in etharp.c and return s8_t where >= 0 means index and < 0 is an err_t.

I've pushe your #1 and fixed it that way.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 12 May 2017 08:50:44 AM UTC, comment #8: 

I done it this way as I didn't want to change the API of the `mdns_resp_add_service()`.

But, if api change isn't a problem, I can return the slot number in an additionnal parameter `int*` (if not NULL) and return ERR_OK at end like current version.

Waiting for your decision to send you a new patch.

Regards,
David

David GIRAULT <dgirault>
Wed 10 May 2017 07:52:26 PM UTC, comment #7: 

That would still mix 2 types. Returning an int. In any case, the return value must match the parameter passed to the remove function!

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 10 May 2017 12:36:25 PM UTC, comment #6: 

Ok. I think I understood. What about just changing `slot` to be an `s8_t` ?

David GIRAULT <dgirault>
Wed 10 May 2017 12:04:18 PM UTC, comment #5: 

> 'err_t' (which is an 'int' ?)


err_t is s8_t by default, not int (see err.h)

> Patches files rebase on latest master (ie. 'ae210967').


Thanks.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 10 May 2017 11:47:52 AM UTC, comment #4: 

Patches files rebase on latest master (ie. 'ae210967').

(file #40646, file #40647)

David GIRAULT <dgirault>
Wed 10 May 2017 11:46:53 AM UTC, comment #3: 

Sorry, I don't understand your comment #2. I can't return an int for function declared to return an int?

- slot >= 0 is valid slot number and .
- all negative value are error code (ERR_MEM if no free slot, ERR_VAL if invalid param).

Should I change return value of the function to 'int' instead of 'err_t' (which is an 'int' ?)

David GIRAULT <dgirault>
Wed 10 May 2017 10:34:15 AM UTC, comment #2: 

From trying to read patch 1 manually: you can't return 'int slot' when the function's return type is 'int'!

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 10 May 2017 10:31:39 AM UTC, comment #1: 

"Could not parse revision. libgit2 returned: revspec '6e8faea' not found"

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 10 May 2017 08:26:00 AM UTC, original submission:  

This patch will add to mdns apps:

- Support for removing service in mdns responder using `mdns_resp_del_service()` and the slotID now returned by the `mdns_resp_add_service()`.

- Support for searching specified services in network through two new functions: `mdns_search_service()` & `mdns_search_stop()`.

David GIRAULT <dgirault>

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dgirault (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-07 dziegel StatusNone => Done
        Open/ClosedOpen => Closed
    2017-05-31 dgirault Attached File- => Added 0001-mdns-added-support-for-searching-services.patch, #40822
    2017-05-10 dgirault Attached File- => Added 0001-mdns-added-support-for-removing-service-in-mdns-resp.patch, #40646
        Attached File- => Added 0002-mdns-added-support-for-searching-services.patch, #40647
    2017-05-10 dgirault Attached File- => Added 0001-mdns-added-support-for-removing-service-in-mdns-resp.patch, #40643
        Attached File- => Added 0002-mdns-added-support-for-searching-services.patch, #40644

    Back to the top


    Powered by Savane 3.5