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

[mdns] fix bug that avahi may publish redundant meshcop services #1104

Merged
merged 2 commits into from
Nov 22, 2021

Conversation

superwhd
Copy link
Contributor

@superwhd superwhd commented Nov 22, 2021

Avahi may rename the meshcop service and publish it when there's a name conflict. However, our current logic has a bug that it may publish redundant meshcop services after some renaming.

For example there are two BRs on the infra link:

  1. BR1 starts. It publishes instance._meshcop._udp.
  2. BR2 starts. It publishes instance #1._meshcop._udp.
  3. BR1 stops. It unpublishes instance._meshcop._udp.
  4. BR2 wants to update the existing meshcop service. However, it actually publishes a new service instance._meshcop._udp. There are two meshcop services existing at the same time both published by BR2.

To fix this bug, we will keep track of the original name in the avahi publisher. When we call PublishService, we should always pass in the original name and let the publisher handle the renaming. In this way we will be able to find the wanted service when updating it or unpublishing it.

The test case is covered in openthread/openthread#7192.

@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #1104 (5a81bc3) into main (7d61390) will increase coverage by 0.00%.
The diff coverage is 58.82%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1104   +/-   ##
=======================================
  Coverage   56.76%   56.77%           
=======================================
  Files          85       85           
  Lines        6352     6358    +6     
=======================================
+ Hits         3606     3610    +4     
- Misses       2746     2748    +2     
Impacted Files Coverage Δ
src/mdns/mdns.hpp 83.33% <ø> (ø)
src/mdns/mdns_avahi.hpp 30.00% <ø> (ø)
src/mdns/mdns_avahi.cpp 45.80% <58.82%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d61390...5a81bc3. Read the comment docs.

Copy link
Member

@simonlingoogle simonlingoogle left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jwhui jwhui changed the title [mdns] fix the bug that avahi may publish redundant meshcop services [mdns] fix bug that avahi may publish redundant meshcop services Nov 22, 2021
Copy link
Member

@jwhui jwhui left a comment

Choose a reason for hiding this comment

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

Thanks! 👍🏼

@jwhui jwhui merged commit d14c225 into openthread:main Nov 22, 2021
Irving-cl pushed a commit to Irving-cl/ot-br-posix that referenced this pull request Dec 7, 2021
…nthread#1104)

Avahi may rename the meshcop service and publish it when there's a
name conflict. However, our current logic has a bug that it may
publish redundant meshcop services after some renaming.

For example there are two BRs on the infra link:
- BR1 starts. It publishes instance._meshcop._udp.
- BR2 starts. It publishes instance openthread#1._meshcop._udp.
- BR1 stops. It unpublishes instance._meshcop._udp.
- BR2 wants to update the existing meshcop service. However, it
  actually publishes a new service instance._meshcop._udp. There are
  two meshcop services existing at the same time both published by
  BR2.

To fix this bug, we will keep track of the original name in the avahi
publisher. When we call PublishService, we should always pass in the
original name and let the publisher handle the renaming. In this way
we will be able to find the wanted service when updating it or
unpublishing it.

GitOrigin-RevId: d14c225
(cherry picked from commit 33eb9b87edade197db47f857836ef1642d74ee33)
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.

None yet

5 participants