Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mbed-os/LwIP changes and fixes in auto-IP for Bonjour Conformance Test #11339

Merged
merged 2 commits into from Aug 29, 2019

Conversation

@sathishm6
Copy link

commented Aug 27, 2019

Description

This PR is to fix the issues in LwIP for AutoIP which is required for passing Bonjour Conformance Test for mDNS. Following gives the summary of the changes/fixes added.

Changes:

  1. Following issues are fixed in LwIP for AutoIP.

    • Fixed bug in max conflict rate limiting: According to RFC section RFC 3927 Section 2.2.1 conflict probe interval should be increased to 60 seconds, once conflict count reaches after MAX_CONFLICTS (i.e., 10) counts. The initial value of 'autoip->tried_llipaddr' is 0. Hence the probe interval (i.e., autoip->ttw) should be increased to 60 secs when 'autoip->tried_llipaddr >= MAX_CONFLICTS'
    • Added code to free 'autoip' client in autoip_stop() API: New 'autoip' client is allocated in autoip_start() API, and the client is not freed during autoip_stop(). This would result in memory leak, if not freed. Updated autoip_stop() API to take care of releasing the memory allocated for 'autoip' client.
  2. Introduced a configurable macro "MBED_CONF_LWIP_DHCP_TIMEOUT" in "lwipopts.h" to configure DHCP timeout based on the usecase requirement. For example: bonjour conformance test would need a DHCP timeout value which is grater than 320 secs to run mDNS probing test to verify protocol compilance of the implementation.

    Tested the fixes using Bonjour Conformance Test tool Version 1.5.0 for IPv4. It has successfully passed Bonjour Conformance Test.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@AnttiKauppila

Release Notes

Changes:
  1. Following issues are fixed in LwIP for AutoIP.
     a) Fixed bug in max conflict rate limitting.
        - According to RFC section RFC 3927 Section 2.2.1 conflict probe interval
          should be increased to 60 seconds, once conflict count reaches after
          MAX_CONFLICTS (i.e., 10) counts.
        - The initial value of 'autoip->tried_llipaddr' is 0. Hence the probe
          interval (i.e., autoip->ttw) should be increased to 60 secs
          when 'autoip->tried_llipaddr >= MAX_CONFLICTS'

     b) Added code to free 'autoip' client in autoip_stop() API.
        - New 'autoip' client is allocated in autoip_start() API, and the client
          is not freed during autoip_stop(). This would result in memory leak
          if not freed.
        - Updated autoip_stop() API to take care of releasing the memory allocated
          for 'autoip' client.
  2. Introduced a configurable macro "MBED_CONF_LWIP_DHCP_TIMEOUT" in "lwipopts.h"
     to configure DHCP timeout based on the usecase requirement. For example:
     bonjour conformance test would need a DHCP timeout value which is grater than
     320 secs to run mDNS probing test to verify protocol compilance of the implementation.
@sathishm6

This comment has been minimized.

Copy link
Author

commented Aug 27, 2019

@AnttiKauppila for review. Please add more reviewers as required. Thanks.

@ciarmcom ciarmcom requested review from AnttiKauppila and ARMmbed/mbed-os-maintainers Aug 27, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Do changes in features/lwipstack/lwip/src/core/ipv4/lwip_autoip.c also live in the upstream repo or we are fixing them here and upstreaming later ?

@sathishm6

This comment has been minimized.

Copy link
Author

commented Aug 27, 2019

@0xc0170 - Hello Martin - I'm pushing the fixes first here in mbed-os. Following this, I plan to push the fix in LwIP repo as well. But the current code base in LwIP repo is little different than what we have here in mbed-os today. However, I am planning to apply these fixes in the new code base on LwIP in it's repo. Thanks.

@@ -94,6 +94,9 @@
static err_t autoip_arp_announce(struct netif *netif);
static void autoip_start_probing(struct netif *netif);

/* static variables */
static u8_t is_autoip_alloced = 0; /* Set when 'struct autoip' is allocated dynamically and assigned to 'netif' */

This comment has been minimized.

Copy link
@AnttiKauppila

AnttiKauppila Aug 28, 2019

Contributor

Why is this needed? Isn't if (autoip == NULL) enough?

This comment has been minimized.

Copy link
@sathishm6

sathishm6 Aug 28, 2019

Author

LwIP provides a way for the caller to assign the static autoip client (structure). This can be achieved by calling autoip_set_struct() API. When the caller allocates the autoip client and assigns it to the netif interface by calling autoip_set_struct() API, LwIP doesn't allocate the autoip client in autoip_start(). Hence, if the autoip client (structure) is allocated by the caller and assigned it to netif interface by calling autoip_set_struct() API, then autoip structure shouldn't be freed in autoip_stop(), otherwise it should be freed during stop. This flag is used to differentiate the above case.

This comment has been minimized.

Copy link
@AnttiKauppila

AnttiKauppila Aug 28, 2019

Contributor

Ok, thanks for the answer, could you fix the naming to ..._allocated

This comment has been minimized.

Copy link
@sathishm6

sathishm6 Aug 28, 2019

Author

Sure, I was trying to keep the name shorter.

This comment has been minimized.

Copy link
@sathishm6

sathishm6 Aug 28, 2019

Author

@AnttiKauppila - Renamed the flag. Committed in SHA#a4aeee941d263e0e5d05c561ca6689e581af214b

This PR is to fix the issues in LwIP for AutoIP which is required for passing Bonjour Conformance Test for mDNS. Following gives the summary of the changes/fixes added.

Changes:

1. Following issues are fixed in LwIP for AutoIP.
- Fixed bug in max conflict rate limiting: According to RFC section RFC 3927 Section 2.2.1 conflict probe interval should be increased to 60 seconds, once conflict count reaches after MAX_CONFLICTS (i.e., 10) counts. The initial value of 'autoip->tried_llipaddr' is 0. Hence the probe interval (i.e., autoip->ttw) should be increased to 60 secs when 'autoip->tried_llipaddr >= MAX_CONFLICTS'
- Added code to free 'autoip' client in autoip_stop() API: New 'autoip' client is allocated in autoip_start() API, and the client is not freed during autoip_stop(). This would result in memory leak, if not freed. Updated autoip_stop() API to take care of releasing the memory allocated for 'autoip' client.

2. Introduced a configurable macro "MBED_CONF_LWIP_DHCP_TIMEOUT" in "lwipopts.h" to configure DHCP timeout based on the usecase requirement. For example: bonjour conformance test would need a DHCP timeout value which is grater than 320 secs to run mDNS probing test to verify protocol compilance of the implementation.

Tested the fixes using Bonjour Conformance Test tool Version 1.5.0 for IPv4. It has successfully passed Bonjour Conformance Test.
@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 28, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 29, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit c1f1b2a into ARMmbed:master Aug 29, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8760 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.