patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9860, mdns: fixes for Bonjour...

 
 

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

patch #9860: mdns: fixes for Bonjour Conformance Tests

Submitter:  David GIRAULT <dgirault>
Submitted:  Tue 15 Oct 2019 11:17:23 AM UTC
   
 
Category:  apps Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  2.2.0

Jump to the original submission

Wed 11 Dec 2019 07:17:09 PM UTC, comment #6: 

Pushed, thanks for your work!

Simon Goldschmidt <goldsimon>
Group administrator
Wed 11 Dec 2019 05:00:18 PM UTC, comment #5: 

Please, see attached new patchset (all in one tar.bz2 file).
All problem fixed and compilation checked with travis script.


(file #48046)

David GIRAULT <dgirault>
Wed 11 Dec 2019 01:06:04 PM UTC, comment #4: 


commentaire #2 :

> Oh, just noticed: there are a few compiler errors:
> - mdns_handle_probe_tiebreaking(): local variable 'mdns' not used
> - mdns_get_service_txt_userdata(): our old C standard wants declarations before code
> - msdn example (now part of our source tree) should be adapted to the changed mdns_name_result_cb_t type ('mdns_example_report')



Hum strange. I'll check with travis.sh script and fix them.

David GIRAULT <dgirault>
Wed 11 Dec 2019 12:02:12 PM UTC, comment #3: 

Hi Simon,

commentaire #1 :

> Thanks for sharing this! And sorry for taking so long to review.
>
> The patches overall look good, however, some comments:
>
> - some typos in commit messages and comments here and there...


I'll try to fix the one I can find.

>
> - patch 1 increases memory usage. This should probably be a config option, and be prefixed correctly (LWIP_MDNS_OUTPACKET_SIZE, not just OUTPACKET_SIZE)


Ok, will rename & move it to mdns_opts.h.

> - patch 4: instead of repeating MDNS_INITIAL_PROBE_DELAY_MS and MDNS_PROBE_DELAY_MS all over the place, could you hide this in two functions (e.g. mdns_resp_restart() and mdns_resp_restart_initial())?


You're right. This is a better solution.

> - patch 6: please improve the description of MDNS_MAX_STORED_PKTS and how it affects memory consumption (e.g. what is this used for, so that people can imagine how big this gets)


Ok.


> - patch 9: this 50ms offset seems kind of random?


You're right. It's a problem. I'll move if as an option in mdns_opts.h with the default value of 250.

>

David GIRAULT <dgirault>
Tue 03 Dec 2019 09:16:53 PM UTC, comment #2: 

Oh, just noticed: there are a few compiler errors:
- mdns_handle_probe_tiebreaking(): local variable 'mdns' not used
- mdns_get_service_txt_userdata(): our old C standard wants declarations before code
- msdn example (now part of our source tree) should be adapted to the changed mdns_name_result_cb_t type ('mdns_example_report')

Simon Goldschmidt <goldsimon>
Group administrator
Tue 03 Dec 2019 09:13:17 PM UTC, comment #1: 

Thanks for sharing this! And sorry for taking so long to review.

The patches overall look good, however, some comments:

- some typos in commit messages and comments here and there...

- patch 1 increases memory usage. This should probably be a config option, and be prefixed correctly (LWIP_MDNS_OUTPACKET_SIZE, not just OUTPACKET_SIZE)
- patch 4: instead of repeating MDNS_INITIAL_PROBE_DELAY_MS and MDNS_PROBE_DELAY_MS all over the place, could you hide this in two functions (e.g. mdns_resp_restart() and mdns_resp_restart_initial())?
- patch 6: please improve the description of MDNS_MAX_STORED_PKTS and how it affects memory consumption (e.g. what is this used for, so that people can imagine how big this gets)
- patch 9: this 50ms offset seems kind of random?


Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Oct 2019 11:17:23 AM UTC, original submission:  

This patchset is all change I need to pass the Apple Bonjour Conformance Tests with success.

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 goldsimon (Posted a comment)
  • -email is unavailable- added by dgirault (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-12-11 goldsimon Planned ReleaseNone 2.2.0
    2019-12-11 goldsimon StatusIn Progress Done
        Open/ClosedOpen Closed
    2019-12-11 dgirault Attached File- Added mdns-bct-fixes.tar.bz2, #48046
    2019-12-03 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2019-10-15 dgirault Attached File- Added 0009-mdns-grow-prob-delay-to-300ms-to-accomodate-WiFi-lat.patch, #47681
        Attached File- Added 0010-mdns-restart-probing-when-IP-addresses-has-changed.patch, #47682
    2019-10-15 dgirault Attached File- Added 0005-mdns-remove-service-TXT-record-from-probe-packets.patch, #47677
        Attached File- Added 0006-mdns-support-for-multi-packet-known-answer-questions.patch, #47678
        Attached File- Added 0007-mdns-handle-tiebreaking-loose-like-conflict.patch, #47679
        Attached File- Added 0008-mdns-abort-packet-analysis-if-conflict-detected.patch, #47680
    2019-10-15 dgirault Attached File- Added 0001-mdns-increase-mDNS-output-packet-size.patch, #47673
        Attached File- Added 0002-mdns-update-probe-conflict-function-to-provide-servi.patch, #47674
        Attached File- Added 0003-mdns-remove-dupplicate-acd_state_enum_t-declaration.patch, #47675
        Attached File- Added 0004-mdns-change-mdns_resp_restart-to-allow-probe-delay-s.patch, #47676

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code