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

fix(mdns): remove same protocol services with different instances #574

Merged

Conversation

DejinChen
Copy link
Contributor

@DejinChen DejinChen commented May 20, 2024

Problem

ESP-Matter may publish services with the same protocol and service type but different instance names. e.g., ''a.matter._tcp" and "b.matter._tcp". When service info changed, we have to remove all "matter._tcp" service through mdns_service_remove(const char *service_type, const char *proto) and re-add new service. But now, the the different instance name services can not be removed at the same time. And re-adding will lead a memory leak problem for the unremoved services with the same subtype.

Change overview:

  1. Modify remove services action to remove the same instance name services.
  2. Verify that the service already has the same subtype before adding.

@DejinChen DejinChen force-pushed the fix/remove_same_protocol_service branch from 1235c28 to 9f70e7e Compare May 20, 2024 06:44
@gabsuren gabsuren added the mdns label May 23, 2024
@gabsuren gabsuren requested review from gabsuren and david-cermak May 24, 2024 11:34
@DejinChen DejinChen force-pushed the fix/remove_same_protocol_service branch from 9f70e7e to 747eb3e Compare May 31, 2024 08:10
Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

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

LGTM, just please fix the CI.

@gabsuren can help you with the AFL host test, probably something missing on the mocking layer.

@DejinChen DejinChen force-pushed the fix/remove_same_protocol_service branch from 747eb3e to 042533a Compare May 31, 2024 08:37
@gabsuren gabsuren merged commit 7437d31 into espressif:master Jun 3, 2024
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants