[patch #9860] mdns: fixes for Bonjour Conformance Tests

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
URL:
  <https://savannah.nongnu.org/patch/?9860>

                 Summary: mdns: fixes for Bonjour Conformance Tests
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: dgirault
            Submitted on: mar. 15 oct. 2019 11:17:23 UTC
                Category: apps
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

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




    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: mar. 15 oct. 2019 11:17:23 UTC  Name:
0001-mdns-increase-mDNS-output-packet-size.patch  Size: 2kio   By: dgirault

<http://savannah.nongnu.org/patch/download.php?file_id=47673>
-------------------------------------------------------
Date: mar. 15 oct. 2019 11:17:23 UTC  Name:
0002-mdns-update-probe-conflict-function-to-provide-servi.patch  Size: 5kio  
By: dgirault

<http://savannah.nongnu.org/patch/download.php?file_id=47674>
-------------------------------------------------------
Date: mar. 15 oct. 2019 11:17:23 UTC  Name:
0003-mdns-remove-dupplicate-acd_state_enum_t-declaration.patch  Size: 2kio  
By: dgirault

<http://savannah.nongnu.org/patch/download.php?file_id=47675>
-------------------------------------------------------
Date: mar. 15 oct. 2019 11:17:23 UTC  Name:
0004-mdns-change-mdns_resp_restart-to-allow-probe-delay-s.patch  Size: 5kio  
By: dgirault

<http://savannah.nongnu.org/patch/download.php?file_id=47676>

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message posté via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Additional Item Attachment, patch #9860 (project lwip):

File name: 0005-mdns-remove-service-TXT-record-from-probe-packets.patch Size:1
KB
   
<https://savannah.nongnu.org/file/0005-mdns-remove-service-TXT-record-from-probe-packets.patch?file_id=47677>

File name: 0006-mdns-support-for-multi-packet-known-answer-questions.patch
Size:7 KB
   
<https://savannah.nongnu.org/file/0006-mdns-support-for-multi-packet-known-answer-questions.patch?file_id=47678>

File name: 0007-mdns-handle-tiebreaking-loose-like-conflict.patch Size:5 KB
   
<https://savannah.nongnu.org/file/0007-mdns-handle-tiebreaking-loose-like-conflict.patch?file_id=47679>

File name: 0008-mdns-abort-packet-analysis-if-conflict-detected.patch Size:0
KB
   
<https://savannah.nongnu.org/file/0008-mdns-abort-packet-analysis-if-conflict-detected.patch?file_id=47680>



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message posté via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Additional Item Attachment, patch #9860 (project lwip):

File name: 0009-mdns-grow-prob-delay-to-300ms-to-accomodate-WiFi-lat.patch
Size:0 KB
   
<https://savannah.nongnu.org/file/0009-mdns-grow-prob-delay-to-300ms-to-accomodate-WiFi-lat.patch?file_id=47681>

File name: 0010-mdns-restart-probing-when-IP-addresses-has-changed.patch
Size:0 KB
   
<https://savannah.nongnu.org/file/0010-mdns-restart-probing-when-IP-addresses-has-changed.patch?file_id=47682>



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message posté via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Update of patch #9860 (project lwip):

                  Status:                    None => In Progress            
             Assigned to:                    None => goldsimon              

    _______________________________________________________

Follow-up 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?



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Follow-up Comment #2, patch #9860 (project lwip):

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')

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Follow-up Comment #3, patch #9860 (project lwip):

Hi Simon,

[comment #1 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.

>

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message posté via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Follow-up Comment #4, patch #9860 (project lwip):


[comment #2 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.


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message posté via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Follow-up Comment #5, patch #9860 (project lwip):

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


(file #48046)
    _______________________________________________________

Additional Item Attachment:

File name: mdns-bct-fixes.tar.bz2         Size:9 KB
    <https://savannah.nongnu.org/file/mdns-bct-fixes.tar.bz2?file_id=48046>



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message posté via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Update of patch #9860 (project lwip):

                  Status:             In Progress => Done                  
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #6:

Pushed, thanks for your work!

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9860] mdns: fixes for Bonjour Conformance Tests

Simon Goldschmidt
Update of patch #9860 (project lwip):

         Planned Release:                    None => 2.2.0                  


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9860>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel