Skip to content

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

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

Merged
merged 2 commits into from
Aug 29, 2019

Conversation

sathishm6
Copy link

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
Copy link
Author

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

@ciarmcom ciarmcom requested review from AnttiKauppila and a team August 27, 2019 05:00
@ciarmcom
Copy link
Member

@sathishm6, thank you for your changes.
@AnttiKauppila @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 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
Copy link
Author

@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' */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@sathishm6 sathishm6 Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@sathishm6 sathishm6 Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I was trying to keep the name shorter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

0xc0170 commented Aug 28, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Aug 29, 2019

Test run: SUCCESS

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants