espressif/esp-protocols
1
0
Fork 1
mirror of https://github.com/espressif/esp-protocols.git synced 2025-12-05 00:29:22 +01:00
Code Issues Packages Projects Releases Wiki Activity
Files
4d8d25a3455e505ec217e34ac8d34cf7cd008c0c
esp-protocols/components/mdns/refactoring_details.md

245 lines
158 KiB
Markdown
Raw Normal View History

feat(mdns): Refactor mdns library (stage #1) The MDNS component has been refactored from a single monolithic file mdns.c into a set of focused modules with clear responsibilities. This restructuring maintains the same functionality while improving code organization, maintainability, and testability. In the stage#2 we will focus on module based tests In the stage#3 we will focus on small scale refators and optimizations
2025-02-07 11:39:11 +01:00
| Original Function | Refactored Function | Concerns |
|------------------|--------------------|---------|
| [mdns_if_from_preset_if](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L136) | [mdns_if_from_preset](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L87) | The function name was changed from `mdns_if_from_preset_if` to `mdns_if_from_preset`, but the logic remains identical. While this appears to be a systematic naming convention change (supported by the evidence that all call sites in the refactored codebase now use the new name), there's a risk that some call sites in the original codebase might not have been updated, or that external code depending on the original function name could break. The functionality itself is preserved, but the interface change could cause linkage or compilation issues if not all dependencies are updated. |
| [esp_netif_from_preset_if](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L151) | [netif_from_preset](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L102) | |
| [_mdns_get_esp_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L179) | [mdns_priv_get_esp_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L118) | The function `esp_netif_from_preset_if()` has been replaced with `netif_from_preset()` in the refactored code. While both functions appear to serve the same purpose of looking up predefined network interfaces, this change could introduce subtle bugs if: <br/> 1. The new function has different error handling behavior <br/> 2. The new function returns different values for edge cases (e.g., when interfaces are not available) <br/> 3. The new function has different performance characteristics that could affect timing-sensitive MDNS operations <br/> <br/> The original `esp_netif_from_preset_if()` was an inline function with a switch statement mapping predefined interface types to actual esp_netif instances. If `netif_from_preset()` has a different implementation or doesn't handle all the same `mdns_predef_if_t` enum values, this could break MDNS functionality for predefined interfaces like STA, AP, or ETH when they become available after initialization. |
| [_mdns_clean_netif_ptr](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L196) | [mdns_priv_netif_disable](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L134) | |
| [_mdns_get_if_from_esp_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L207) | [get_if_from_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L144) | The function `esp_netif_from_preset_if` was renamed to `netif_from_preset` in the refactored code. While both functions appear to serve the same purpose (converting a predefined interface enum to an esp_netif pointer), this change could introduce subtle bugs if the implementation of `netif_from_preset` differs from the original `esp_netif_from_preset_if` function, particularly in error handling, return values for unsupported interface types, or behavioral differences in edge cases. |
| [_str_null_or_empty](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L224) | [mdns_utils_str_null_or_empty](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.h#L38) | The refactoring introduced a potential compilation issue. The original function `_str_null_or_empty` was defined as `static inline` within `mdns.c`, making it only visible within that translation unit. The refactored version `mdns_utils_str_null_or_empty` is also `static inline` but in `mdns_utils.h`, which means it needs to be properly included in all files that use it. From the search results, I can see that `mdns_browser.c`, `mdns_debug.c`, and `mdns_responder.c` are using the new function, but I cannot verify if the original `mdns.c` file has been updated to include `mdns_utils.h` and replace all calls to `_str_null_or_empty` with `mdns_utils_str_null_or_empty`. If the original `mdns.c` still contains calls to the old function name without including the new header, this will cause compilation failures. |
| [_mdns_mangle_name](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L232) | [mangle_name](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L138) | |
| [_mdns_service_match](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L272) | [mdns_utils_service_match](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L121) | The refactored function adds a null check for the 'srv' parameter (!srv), which changes the behavior from the original function. In the original implementation, callers could potentially pass NULL for 'srv', which would cause a segmentation fault when accessing srv->hostname. The refactored version now explicitly checks for NULL and returns false, which may mask underlying bugs in calling code that should be fixed rather than silently handled. Additionally, the function visibility changed from static to public, which could break encapsulation if this was intended to be an internal helper function. |
| [_mdns_get_service_item](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L291) | [mdns_utils_get_service_item](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L131) | The refactored function changed from static to non-static visibility and now uses mdns_priv_get_services() instead of directly accessing _mdns_server->services. This could introduce subtle bugs related to: <br/> 1. Thread safety - the original function was likely called within MDNS_SERVICE_LOCK/UNLOCK contexts, but the refactored version's access pattern through mdns_priv_get_services() may not provide the same guarantees <br/> 2. Encapsulation - making the function non-static exposes internal service lookup logic that was previously hidden <br/> 3. Dependency on mdns_utils_service_match having identical behavior to _mdns_service_match, which needs verification <br/> 4. Potential null pointer issues if mdns_priv_get_services() returns NULL when _mdns_server->services would not be NULL |
| [_mdns_get_service_item_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L303) | [get_service_item_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L39) | |
| [mdns_get_host_item](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L321) | [get_host_item](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L240) | The refactored get_host_item function introduces a potential NULL return when the hostname matches the server's hostname or is NULL. In the original implementation, &_mdns_self_host was always returned (a guaranteed non-NULL address), but the refactored version calls mdns_priv_get_self_host() which can return NULL if s_server is NULL. This could break callers that assume non-NULL returns for self-host cases, potentially causing segmentation faults or incorrect behavior in service discovery and host resolution. |
| [_mdns_can_add_more_services](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L336) | [can_add_more_services](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L129) | |
| [_mdns_send_rx_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L354) | [send_rx_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_networking_lwip.c#L48) | The refactored function 'send_rx_action' replaces the direct xQueueSend call with mdns_priv_queue_action, but the implementation of mdns_priv_queue_action is not visible in the provided context. This introduces a potential subtle bug if mdns_priv_queue_action does not properly handle the queue operation with the same blocking/non-blocking behavior (original used (TickType_t)0 timeout) or if it returns incorrect boolean values that don't match the expected ESP_OK/ESP_ERR_NO_MEM semantics. Additionally, the function was changed from global to static scope, which could break external callers if not properly addressed in the refactoring. |
| [_mdns_get_default_instance_name](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L373) | [get_default_instance_name](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L175) | The refactored function introduces potential subtle bugs due to the separation of concerns. Specifically: <br/> 1. The helper functions mdns_priv_get_instance() and mdns_priv_get_global_hostname() may have different NULL-checking behavior than the original direct _mdns_server access <br/> 2. There could be thread safety issues if s_server (the refactored equivalent of _mdns_server) is accessed without proper synchronization <br/> 3. The behavior when s_server is NULL may not exactly match the original behavior, particularly in edge cases <br/> 4. The replacement of _str_null_or_empty with mdns_utils_str_null_or_empty could have different implementations that handle edge cases differently |
| [_mdns_get_service_instance_name](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L389) | [mdns_utils_get_service_instance_name](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L192) | The refactored function get_default_instance_name() can return NULL when both mdns_priv_get_instance() and mdns_priv_get_global_hostname() return NULL or empty strings, whereas the original _mdns_get_default_instance_name() appears to always return a non-NULL string. This behavioral change could lead to null pointer dereferences in callers that expect a valid string, as evidenced by the original usage patterns where the return value is used directly in string operations without null checks. |
| [_mdns_instance_name_match](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L398) | [mdns_utils_instance_name_match](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L202) | The refactored function `mdns_utils_instance_name_match` calls `get_default_instance_name()` which uses `mdns_priv_get_instance()` internally, while the original function `_mdns_instance_name_match` used `_mdns_get_default_instance_name()` that directly accessed `_mdns_server->instance`. The behavioral equivalence depends on whether `mdns_priv_get_instance()` returns the same value as the original direct server access, particularly in edge cases where the server state might be inconsistent or during initialization/teardown phases. |
| [_mdns_service_match_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L409) | [mdns_utils_service_match_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L162) | The refactored function changed from static to public scope (removed static keyword), which could break encapsulation and allow unintended external access to internal service matching logic. Additionally, the helper function `mdns_utils_str_null_or_empty` might have different behavior than the original `_str_null_or_empty` if it was reimplemented during the utility extraction. |
| [_mdns_get_service_item_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L422) | [mdns_utils_get_service_item_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L143) | |
| [_mdns_read_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L452) | [mdns_utils_read_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L18) | The refactored function mdns_utils_parse_fqdn uses a static buffer (static char buf[MDNS_NAME_BUF_LEN]) which could cause thread safety issues in a multi-threaded environment or reentrancy problems if the function is called recursively. Additionally, the memory movement operations using memmove in mdns_utils_parse_fqdn appear to be incorrectly calculating the offsets - the destination and source calculations seem to assume the mdns_name_t structure fields are exactly MDNS_NAME_BUF_LEN bytes apart, which may not be correct if the structure has padding or different field sizes. |
| [_mdns_set_u16](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L512) | ??? | |
| [_mdns_append_u8](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L530) | [mdns_utils_append_u8](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.h#L147) | |
| [_mdns_append_u16](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L549) | [mdns_utils_append_u16](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L262) | |
| [_mdns_append_u32](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L568) | [append_u32](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L39) | The bounds check in the refactored function uses `(*index + sizeof(uint32_t)) >= MDNS_MAX_PACKET_SIZE` while the original used `(*index + 3) >= MDNS_MAX_PACKET_SIZE`. Although `sizeof(uint32_t)` should typically be 4, this creates a potential portability issue if compiled on a system where `uint32_t` has a different size. Additionally, the refactored function now depends on `mdns_utils_append_u8` which has different error handling - it returns 0 on bounds violation but doesn't prevent the index from being incremented in subsequent calls, potentially causing inconsistent state. |
| [_mdns_append_type](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L590) | [append_type](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L61) | The refactored function uses inconsistent function naming: `mdns_utils_append_u16` and `append_u32` (without the `mdns_utils_` prefix). This suggests either a missing utility function implementation or inconsistent refactoring that could lead to compilation errors if `append_u32` is not properly defined in the same context. Additionally, the buffer size calculation changed from hardcoded 10 to using sizeof operations, which while functionally equivalent, introduces potential maintenance complexity. |
| [_mdns_append_string_with_len](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L622) | [append_string](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L103) | The refactored function `append_string` introduces a subtle bug by replacing the explicit length parameter with `strlen(string)`. This breaks compatibility with: <br/> 1. Non-null-terminated strings that were previously supported <br/> 2. Binary data that might contain embedded null bytes <br/> 3. Callers who want to control the exact length written (important for DNS label encoding) <br/> 4. Embedded systems use cases where string length might be known without scanning <br/> <br/> Additionally, the return value behavior has changed from returning the number of bytes written (len + 1) to potentially returning 0 on failure, which may break error handling logic in callers expecting the original return value pattern. |
| [_mdns_append_string](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L642) | [append_string](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L103) | The refactored function `append_single_str` has a critical semantic change in its return value. While the original `_mdns_append_string` returns the length of added data (string length + 1) on success, `append_single_str` returns the updated packet index. This could break calling code that relies on the original return value semantics for length calculations or error checking. Additionally, `append_single_str` introduces an extra error check for `mdns_utils_append_u8` that wasn't present in the original, which changes the error handling behavior. |
| [append_one_txt_record_entry](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L665) | [mdns_priv_append_one_txt_record_entry](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L3399) | The boundary check condition `(*index + len + 1) >= MDNS_MAX_PACKET_SIZE` in both original and refactored code may be incorrect. It should likely use `>` instead of `>=` to prevent buffer overflow when `*index + len + 1` exactly equals `MDNS_MAX_PACKET_SIZE`. This would write one byte beyond the allocated buffer size. The refactoring preserved this potential off-by-one error rather than fixing it. |
| [append_single_str](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L686) | [append_single_str](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L430) | The refactored append_single_str function now calls mdns_utils_append_u8 instead of _mdns_append_u8, but the return value check is incorrect. The original _mdns_append_u8 likely returned 0 on failure, but the new mdns_utils_append_u8 returns 1 on success and 0 on failure. However, the refactored code checks "if (!mdns_utils_append_u8(...))" which is correct for the new return semantics, but there's a potential issue: the original function returned the final packet index on success, while the new helper returns a boolean. This could cause issues if any calling code depends on the specific return value semantics beyond just success/failure checking. |
| [append_fqdn_dots](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L711) | [append_fqdn_dots](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L455) | The refactored append_fqdn_dots function in Result 1 has a subtle bug: it always appends "arpa" suffix regardless of the input domain, whereas the original function's logic suggests this should only be used for reverse DNS queries. The function also ignores the 'last' parameter completely, which may indicate missing functionality for handling non-reverse DNS queries. Additionally, the hardcoded "arpa" suffix could cause incorrect FQDN formatting for regular forward DNS queries. |
| [_mdns_append_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L750) | [append_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L667) | The refactored function calls `append_string` instead of `_mdns_append_string`, but the search results show no implementation of `append_string` in the refactored codebase. This suggests the function may be missing, which would cause linking errors or runtime failures. Additionally, the recursive call to `append_fqdn` relies on this potentially missing function, creating a dependency chain that may be broken. |
| [_mdns_append_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L823) | [append_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L781) | |
| [_mdns_append_subtype_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L873) | ??? | |
| [_mdns_append_sdptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L918) | [append_sdptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1038) | |
| [_mdns_append_txt_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L968) | [append_txt_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L974) | The refactored function uses renamed helper functions (mdns_utils_get_service_instance_name, append_fqdn, append_type, mdns_priv_append_one_txt_record_entry, set_u16) and constants (MDNS_UTILS_DEFAULT_DOMAIN) without verification that these have identical behavior and error handling as their original counterparts. Specifically, if any of these helper functions now have different buffer bounds checking, error return values, or memory management behavior, it could introduce subtle bugs in packet construction, buffer overflow protection, or error propagation. |
| [_mdns_append_srv_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1032) | [append_srv_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L902) | The refactored function replaces direct struct member access `_mdns_server->hostname` with a function call `mdns_priv_get_global_hostname()`. This could introduce subtle bugs if: <br/> 1. The function returns NULL when the original struct member was guaranteed to be non-NULL <br/> 2. The function has different error handling semantics <br/> 3. The function introduces side effects or performance overhead not present in the original <br/> 4. The function's behavior differs in edge cases (e.g., during initialization/shutdown) <br/> <br/> Additionally, the function name changes from `_mdns_append_u16` to `mdns_utils_append_u16` and similar utility function renames could introduce behavioral differences if the implementations are not functionally equivalent. |
| [_mdns_append_a_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1105) | [append_a_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1089) | The boundary check condition `(*index + 3) >= MDNS_MAX_PACKET_SIZE` is preserved from the original but appears to be incorrect. When *index equals MDNS_MAX_PACKET_SIZE - 3, the condition evaluates to true (preventing the write), but the code needs to write 4 bytes starting from *index. This means the valid range should be *index <= MDNS_MAX_PACKET_SIZE - 4. The condition should likely be `(*index + 4) > MDNS_MAX_PACKET_SIZE` or `(*index > MDNS_MAX_PACKET_SIZE - 4)` to properly prevent buffer overflow. |
| [_mdns_append_aaaa_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1157) | [append_aaaa_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1141) | The refactored function uses renamed utility functions (append_fqdn, append_type, set_u16, mdns_utils_str_null_or_empty) and constants (MDNS_UTILS_DEFAULT_DOMAIN) without verification that they maintain identical behavior to the original implementations. Specifically, if any of these helper functions now have different error handling, boundary checking, or return value semantics, it could cause subtle bugs in AAAA record construction. The domain name change from MDNS_DEFAULT_DOMAIN to MDNS_UTILS_DEFAULT_DOMAIN could also affect DNS resolution if the values differ. |
| [_mdns_append_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1201) | [append_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L734) | The refactored function uses `append_fqdn` and `mdns_utils_append_u16` which may have different error handling behavior than the original `_mdns_append_fqdn` and `_mdns_append_u16` functions. Specifically, the bounds checking in `mdns_utils_append_u16` returns 0 on error, but the original function's behavior is unknown. Additionally, the cumulative length calculation in the refactored function could be incorrect if the utility functions return 0 on error, as it would add 0 to `part_length` instead of properly handling the error case. |
| [_mdns_get_other_if](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1242) | [mdns_priv_netif_get_other_interface](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L73) | |
| [_mdns_if_is_dup](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1253) | [mdns_priv_pcb_check_for_duplicates](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L89) | The refactored function uses a different data structure (s_pcbs) and interface lookup mechanism (s_esp_netifs[tcpip_if].duplicate) compared to the original (_mdns_server->interfaces[...].pcbs and _mdns_get_other_if). Without verification that these data structures are properly initialized and contain equivalent state information, there's a risk of accessing uninitialized memory or incorrect duplicate detection. Additionally, the refactored code assumes MDNS_IP_PROTOCOL_MAX equals 2 (covering both IPv4 and IPv6), which may not match the original implementation's explicit protocol checks. |
| [_ipv6_address_is_zero](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1273) | [mdns_utils_ipv6_address_is_zero](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L249) | |
| [_mdns_append_host_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1286) | [append_host_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1182) | The refactored append_answer function introduces new host validation logic that checks if the host is valid (either self-host or in the hosts list) before processing. This validation was not present in the original _mdns_append_host_answer function and could potentially block legitimate host answers if the validation logic is incorrect or if there are timing issues with the host list management. Additionally, the return value semantics have changed - the original function returned the count of successfully appended records, while append_answer returns different values (0, 1, 2, count) depending on the context, which could break the calling code's expectations. |
| [_mdns_append_reverse_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1318) | [append_reverse_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L485) | The refactored function uses `mdns_priv_get_self_host()->hostname` without checking if `mdns_priv_get_self_host()` returns NULL. In the original code, `_mdns_self_host.hostname` was accessed directly, suggesting `_mdns_self_host` was a global struct. The new access pattern through a function call introduces a potential null pointer dereference if the function returns NULL when the self host is not properly initialized. |
| [_mdns_append_service_ptr_answers](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1350) | [append_service_ptr_answers](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L870) | |
| [_mdns_append_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1378) | [append_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1215) | The refactoring introduces potential behavioral changes in the IPv4 and IPv6 address handling logic. Specifically: <br/> <br/> 1. The original code checked `_mdns_server->interfaces[tcpip_if].pcbs[MDNS_IP_PROTOCOL_V4].state != PCB_DUP` and `_mdns_server->interfaces[tcpip_if].pcbs[MDNS_IP_PROTOCOL_V6].state != PCB_DUP`, but the refactored code uses `!mdns_priv_pcb_is_duplicate(tcpip_if, MDNS_IP_PROTOCOL_V4)` and `!mdns_priv_pcb_is_duplicate(tcpip_if, MDNS_IP_PROTOCOL_V6)`. The logic appears inverted - the original checks if state is NOT PCB_DUP, while the new function name suggests it checks if IS duplicate. <br/> <br/> 2. The original used `_mdns_if_is_dup(tcpip_if)` while the refactored uses `mdns_priv_pcb_check_for_duplicates(tcpip_if)`. These appear to be different checks with potentially different semantics. <br/> <br/> 3. The IPv6 zero address check changed from `_ipv6_address_is_zero(if_ip6s[i])` to `mdns_utils_ipv6_address_is_zero(if_ip6s[i])`. Without seeing the implementation of both functions, we cannot guarantee identical behavior. <br/> <br/> These changes could affect when additional interface addresses are appended and when IPv6 addresses are considered valid, potentially breaking the duplicate interface handling logic and IPv6 address validation. |
| [_mdns_dispatch_tx_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1490) | [mdns_priv_dispatch_tx_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1329) | The refactoring changed the function from static to global visibility (mdns_priv_dispatch_tx_packet), which breaks encapsulation and could allow unintended external calls to this internal packet dispatch function. Additionally, the static buffer 'packet[MDNS_MAX_PACKET_SIZE]' remains, creating potential thread safety issues if multiple threads call this function concurrently, as they would share the same buffer. The debug functionality extraction to DBG_TX_PACKET macro and mdns_debug_tx_packet function appears correct, but without seeing the macro definition, we cannot verify if debug code is properly conditionally compiled. |
| [_mdns_free_tx_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1559) | [mdns_priv_free_tx_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L187) | |
| [_mdns_schedule_tx_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1588) | [mdns_priv_send_after](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1617) | The refactored function mdns_priv_send_after has changed from static to non-static visibility, which could lead to unintended external access to the internal packet scheduling mechanism. Additionally, the global queue variable has been renamed from _mdns_server->tx_queue_head to s_tx_queue_head, but without seeing the broader context of how s_tx_queue_head is declared and managed, there may be synchronization issues if multiple threads can access this queue concurrently. The original function was tightly coupled with the _mdns_server structure, and this refactoring appears to separate the queue management from the server context without clear evidence of proper synchronization mechanisms being added. |
| [_mdns_clear_tx_queue_head](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1611) | [mdns_priv_clear_tx_queue](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L2107) | The refactored function uses a global variable `s_tx_queue_head` instead of accessing it through `_mdns_server->tx_queue_head`. This changes the data structure from a server-specific queue to a global queue, which could introduce race conditions in multi-server scenarios or break the encapsulation of server state. Additionally, the function name change suggests it's now part of a different module (mdns_send.c), which may indicate architectural changes that need verification across the entire codebase. |
| [_mdns_clear_pcb_tx_queue_head](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1627) | [mdns_priv_clear_tx_queue_if](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1656) | |
| [_mdns_get_next_pcb_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1655) | [mdns_priv_get_next_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L2294) | The refactored function changed from accessing `_mdns_server->tx_queue_head` to `s_tx_queue_head`, which represents a change in global state management. This could introduce subtle bugs if: <br/> 1. `s_tx_queue_head` is not properly initialized or synchronized with the rest of the system <br/> 2. Other parts of the codebase still reference `_mdns_server->tx_queue_head` inconsistently <br/> 3. The queue management operations (add/remove packets) use different queue head variables in different modules <br/> <br/> Additionally, the function visibility changed from static to non-static, which could break encapsulation and allow unintended external access to internal queue operations. |
| [_mdns_remove_scheduled_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1670) | [mdns_priv_remove_scheduled_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L2558) | The refactored function changed from static to non-static scope while maintaining the pattern of creating a local mdns_srv_item_t variable when service is NULL. This could lead to undefined behavior if the function is called from multiple threads or contexts, as the local variable's address is used for comparison in the linked list traversal. Additionally, the global variable access changed from _mdns_server->tx_queue_head to s_tx_queue_head, which may have different synchronization guarantees. |
| [_mdns_dealloc_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1704) | [mdns_priv_dealloc_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L2408) | The refactored function `mdns_priv_dealloc_answer` has a potential bug when `service` parameter is NULL. In the original function, when `service` is NULL, it creates a local `mdns_srv_item_t s` with NULL pointers and uses `service = &s`, then compares `d->service == service->service`. However, `service->service` will be NULL in this case, so it's comparing `d->service == NULL`. This logic is preserved in the refactored version, but the comparison `d->service == service->service` when `service` points to a stack-allocated structure with NULL members could be confusing and error-prone. The function appears to be trying to match answers where the service pointer is NULL, but this pattern is fragile and depends on the caller's understanding of when to pass NULL vs an actual service. |
| [_mdns_alloc_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1733) | [mdns_priv_create_answer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L1053) | The refactored function `mdns_priv_create_answer` has lost the `static` qualifier that was present in the original `_mdns_alloc_answer` function. This changes the function's visibility from file-scope to global scope, which could lead to unintended external access and potential linkage issues. Additionally, the function name change from `_mdns_alloc_answer` to `mdns_priv_create_answer` suggests it should remain private, but removing the `static` qualifier contradicts this intention. |
| [_mdns_alloc_packet_default](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1763) | [mdns_priv_alloc_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L2698) | The function visibility has changed from static to non-static (exported), which could break encapsulation and allow unintended external access to this internal packet allocation function. This may lead to architectural issues where external code could bypass proper MDNS APIs and directly manipulate packet allocation, potentially causing memory management issues or protocol violations. |
| [_mdns_create_answer_from_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1789) | [create_answer_from_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L255) | The refactored function uses `mdns_priv_get_self_host()` instead of comparing directly with `&_mdns_self_host`, which could introduce subtle behavioral differences if the function doesn't return the expected reference. Additionally, the MDNS_TYPE_SDPTR case consistently uses `false` for the `send_flush` parameter in both versions, but this might be an existing issue that wasn't addressed during refactoring. |
| [_mdns_create_answer_from_hostname](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1824) | [create_answer_from_hostname](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L292) | |
| [_mdns_service_match_ptr_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1834) | [service_match_ptr_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L302) | The refactored function uses renamed helper functions (mdns_utils_service_match and mdns_utils_get_service_instance_name) without verification that they maintain identical behavior to the original _mdns_service_match and _mdns_get_service_instance_name functions. If these helper functions have different error handling, null checking, or return value semantics, it could introduce subtle bugs in service discovery logic. |
| [_mdns_create_answer_from_parsed_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L1862) | [mdns_priv_create_answer_from_parsed_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L3276) | The refactored function has changed from static to non-static visibility, which could break encapsulation and allow unintended external access. Additionally, the function `mdns_priv_get_services()` replaces direct access to `mdns_server->services`, but without seeing its implementation, we cannot verify it returns the same service list with proper synchronization. The `get_host_item()` function also replaces `mdns_get_host_item()` - if these have different implementations, it could affect host matching logic and service discovery behavior. |
| [_mdns_question_exists](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2009) | [question_exists](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L377) | |
| [_mdns_append_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2023) | [append_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L326) | The refactored `append_host_list` function contains a potential linked list traversal bug. The original code in `_mdns_append_host_list` correctly traversed the host list by advancing the pointer after processing each host. However, the refactored version advances `host = host->next` before calling `append_host`, which means: <br/> 1. The first host in the list is skipped entirely <br/> 2. The last iteration will attempt to access `host->next` which could be NULL, leading to potential NULL pointer dereference in `append_host` <br/> 3. This could result in missing host entries in MDNS responses <br/> <br/> The correct pattern should be: <br/> ```c <br/> while (host != NULL) { <br/> if (!append_host(destination, host, flush, bye)) { <br/> return false; <br/> } <br/> host = host->next; <br/> } <br/> ``` |
| [_mdns_append_host_list_in_services](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2034) | [mdns_priv_append_host_list_in_services](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L1834) | The refactored function changes the hostname lookup mechanism from `_mdns_server->hostname` to `mdns_priv_get_global_hostname()`. This could introduce subtle bugs if: <br/> 1. The global hostname differs from the server hostname in certain contexts <br/> 2. The `mdns_priv_get_global_hostname()` function has different initialization timing or state dependencies <br/> 3. There are race conditions where the global hostname changes during packet construction <br/> <br/> Additionally, the function dependencies have changed from `mdns_get_host_item` and `_mdns_append_host` to `get_host_item` and `append_host`. Without verifying these functions have identical behavior, there's risk of: <br/> - Different error handling semantics <br/> - Changed memory management patterns <br/> - Modified host lookup logic |
| [_mdns_append_host_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2053) | [append_host_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L356) | The refactored append_host_list function has a critical bug in the while loop logic. The original code correctly processes the current host before advancing to the next one (host = host->next at the end of the loop). The refactored version advances the pointer before processing (host = host->next at the beginning of the loop), which causes it to skip the first host in the list and potentially access invalid memory when the list is empty or has only one element. |
| [_mdns_append_host_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2071) | [append_host_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L391) | The refactored function uses MDNS_UTILS_DEFAULT_DOMAIN instead of MDNS_DEFAULT_DOMAIN, which could be a different constant value. Additionally, it calls question_exists instead of _mdns_question_exists, which might have different implementation behavior. The related function append_host_questions_for_services now uses mdns_priv_get_global_hostname() instead of directly accessing _mdns_server->hostname, which could introduce different behavior if the global hostname retrieval differs from the direct struct access. |
| [_mdns_append_host_questions_for_services](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2094) | [append_host_questions_for_services](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L414) | The refactored function introduces a subtle bug in the append_host_question helper function. The original implementation did not check for duplicate questions before adding them to the queue, but the refactored version now includes a question_exists check that frees the allocated memory if a duplicate is found, yet still returns true. This means: <br/> 1. Memory is allocated but potentially leaked if duplicates exist (malloc followed by free without being added to queue) <br/> 2. The function returns true even when the question wasn't actually added to the questions list <br/> 3. This could lead to inconsistent state where callers think questions were added but they weren't <br/> Additionally, the change from direct _mdns_server->hostname access to mdns_priv_get_global_hostname() and from _str_null_or_empty to mdns_utils_str_null_or_empty could introduce behavioral differences if these functions don't have identical implementations. |
| [_mdns_create_probe_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2112) | [mdns_priv_create_probe_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L887) | The function `question_exists` (without the `mdns_priv_` prefix) appears to be a different implementation than the original `_mdns_question_exists`. This could introduce subtle bugs if the new function has different behavior for duplicate question detection, potentially causing incorrect packet construction or memory leaks. Additionally, the constant `MDNS_UTILS_DEFAULT_DOMAIN` may have a different value than the original `MDNS_DEFAULT_DOMAIN`, which could affect DNS query formatting and service discovery. |
| [_mdns_create_announce_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2166) | [mdns_priv_create_announce_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L1256) | The refactored function uses different underlying helper functions (mdns_priv_alloc_packet, mdns_priv_create_answer, mdns_priv_free_tx_packet, mdns_priv_append_host_list_in_services) compared to the original (_mdns_alloc_packet_default, _mdns_alloc_answer, _mdns_free_tx_packet, _mdns_append_host_list_in_services). While the function signatures appear identical, the inability to find these renamed functions in the search results suggests they may have different implementations or behaviors that could introduce subtle bugs, particularly around memory management, error handling, or protocol compliance. The refactoring assumes functional equivalence of these underlying components without verification. |
| [_mdns_create_announce_from_probe](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2196) | [mdns_priv_create_announce_from_probe](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L1417) | The function `mdns_get_host_item` from the original code has been replaced with `get_host_item` without the `mdns_` prefix. This could indicate either an incomplete refactoring where the prefix was accidentally omitted, or a different function with potentially different behavior. If `get_host_item` is not the correct replacement or has different memory management semantics, it could lead to null pointer dereferences or incorrect host item lookups. |
| [_mdns_pcb_send_bye](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2236) | [mdns_priv_send_bye](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1505) | The refactored function mdns_priv_pcb_send_bye_service introduces a new global hostname check using mdns_utils_str_null_or_empty(mdns_priv_get_global_hostname()) that wasn't present in the original code. This could prevent goodbye packets from being sent when the global hostname is empty, potentially leaving stale service records in the network. The original implementation would send goodbye packets regardless of hostname state, which may be the intended behavior for proper service cleanup. |
| [_mdns_init_pcb_probe_new_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2260) | [init_probe_new_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L339) | The refactoring introduces a potential stack overflow vulnerability in mdns_priv_init_pcb_probe where a Variable Length Array (VLA) `new_probe_services[len]` is allocated on the stack without bounds checking. If `len` is large, this could cause stack exhaustion. Additionally, the global hostname check in mdns_priv_init_pcb_probe may prematurely set PCB state to RUNNING without probing services, potentially skipping necessary service discovery steps. |
| [_mdns_init_pcb_probe_new_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2260) | [mdns_priv_init_pcb_probe](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_pcb.c#L2478) | The refactoring introduces a potential stack overflow vulnerability in mdns_priv_init_pcb_probe where a Variable Length Array (VLA) `new_probe_services[len]` is allocated on the stack without bounds checking. If `len` is large, this could cause stack exhaustion. Additionally, the global hostname check in mdns_priv_init_pcb_probe may prematurely set PCB state to RUNNING without probing services, potentially skipping necessary service discovery steps. |
| [_mdns_init_pcb_probe](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2316) | [mdns_priv_init_pcb_probe](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_pcb.c#L2478) | The refactored code introduces a potential stack overflow vulnerability by using a Variable Length Array (VLA) `new_probe_services[len]` without bounds checking. In the original code, the VLA usage was the same, but the refactoring context doesn't show if `len` is properly validated elsewhere. Large `len` values could exhaust the stack, especially in embedded environments. Additionally, the refactored `init_probe_new_service` function has complex memory management logic that could leak memory if `mdns_priv_create_probe_packet` returns NULL, though this appears consistent with the original design. |
| [_mdns_restart_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2356) | [restart_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L127) | The refactored function uses `mdns_priv_get_services()` instead of direct access to `_mdns_server->services`, which changes the data access pattern. While this might be intentional encapsulation, it could introduce subtle bugs if: <br/> 1. `mdns_priv_get_services()` returns a different service list than the original global variable <br/> 2. The function is called multiple times (line 132 and 140) and the service list changes between calls <br/> 3. The service counting logic becomes inconsistent if services are added/removed during execution <br/> <br/> Additionally, the function name changed from `_mdns_restart_pcb` to `restart_pcb`, which may affect visibility and linkage if this was previously a static function. |
| [_mdns_send_bye](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2382) | [mdns_priv_pcb_send_bye_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L429) | The refactored function uses a different PCB state access pattern: original code accessed `_mdns_server->interfaces[i].pcbs[j].state` while refactored code uses `s_pcbs[i][j].state`. This structural change could introduce subtle bugs if the PCB state management or initialization logic differs between the two implementations, potentially leading to incorrect state checks or accessing uninitialized memory. |
| [_mdns_send_bye_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2401) | [mdns_priv_send_bye_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L1681) | The refactored function changed from static to non-static visibility, which could break encapsulation and allow unintended external access. Additionally, the static buffer 'pkt' remains in the refactored code, creating potential race conditions in multi-threaded environments since multiple instances could overwrite the same buffer. |
| [_mdns_announce_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2451) | [mdns_priv_pcb_announce](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_pcb.c#L861) | The refactored function accesses PCB state through a different global structure (s_pcbs[][] instead of _mdns_server->interfaces[].pcbs[]), which could lead to state inconsistencies if the initialization or synchronization of these structures differs. Additionally, the hostname check now uses mdns_priv_get_global_hostname() instead of direct access to _mdns_server->hostname, which may return different values if the global hostname management was modified during refactoring. |
| [_mdns_probe_all_pcbs](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2494) | [mdns_priv_probe_all_pcbs](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_pcb.c#L2342) | The refactored function accesses PCB data through `s_pcbs[i][j]` instead of `_mdns_server->interfaces[i].pcbs[j]`, which represents a significant architectural change in data structure organization. This could introduce subtle bugs if: <br/> <br/> 1. The `s_pcbs` array is not properly initialized or synchronized with the original server state <br/> 2. The memory layout or indexing differs between the two data structures <br/> 3. There are race conditions in accessing the global `s_pcbs` vs the server-managed PCB structures <br/> <br/> Additionally, the helper function `mdns_priv_init_pcb_probe` (shown in Result 3) has been substantially refactored with new logic for duplicate service detection and different probing state handling, which may change the probing behavior in edge cases where services are already being probed. |
| [_mdns_announce_all_pcbs](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2516) | [announce_all_pcbs](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L150) | The refactored function `announce_all_pcbs` calls `mdns_priv_pcb_announce` which introduces state-dependent behavior that wasn't present in the original implementation. In the PCB_RUNNING state, there's a global hostname check that can cause the function to return early without any announcement if the hostname is null or empty. This could lead to silent failures where services aren't announced when expected, particularly during system initialization or hostname changes. The original implementation would always attempt to announce regardless of PCB state or hostname status. |
| [_mdns_send_final_bye](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2529) | [send_final_bye](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L163) | The refactored function send_final_bye calls mdns_priv_pcb_send_bye_service which adds a hostname validation check that wasn't present in the original function. This could prevent bye packets from being sent when the global hostname is empty/null, potentially leaving stale service records in the network. Additionally, the new implementation iterates through multiple interfaces/protocols and checks PCB state, which may change the multicast behavior compared to the original single-call approach. |
| [_mdns_send_bye_all_pcbs_no_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2554) | [send_bye_all_pcbs_no_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L188) | The refactored function `send_bye_all_pcbs_no_instance` now calls `mdns_priv_pcb_send_bye_service` which includes an additional global hostname check that wasn't present in the original implementation. This could potentially prevent bye packets from being sent when the global hostname is empty, even though services without instances should still be able to send bye packets independently of the hostname state. This represents a behavioral change that might affect service shutdown behavior. |
| [_mdns_restart_all_pcbs_no_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2582) | [mdns_priv_restart_all_pcbs_no_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L213) | |
| [_mdns_restart_all_pcbs](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2610) | [mdns_priv_restart_all_pcbs](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L238) | The refactored function changed from static to non-static visibility, which could lead to unintended external access and linkage issues. Additionally, the global variable changed from _mdns_server to s_server, and function calls changed from _mdns_clear_tx_queue_head() to mdns_priv_clear_tx_queue() and _mdns_probe_all_pcbs() to mdns_priv_probe_all_pcbs(). Without verification that these are functionally equivalent, there's risk of behavioral changes or memory management issues. |
| [_mdns_allocate_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2643) | [allocate_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L269) | The refactored function `allocate_txt` has a subtle memory leak bug. In the original function, when `mdns_mem_strdup(txt[i].value)` fails, it properly cleans up by freeing both `new_item->key` and `new_item` before breaking. However, in the refactored version, when `mdns_mem_strdup(txt[i].value)` fails, it only frees `(char *)new_item->key` and `new_item`, but any previously allocated items in the linked list (`new_txt`) remain allocated and are not cleaned up. This creates a memory leak where partial allocations are not properly rolled back when allocation failures occur mid-loop. |
| [_mdns_free_linked_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2682) | [free_linked_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L308) | The refactored function free_linked_txt casts t->key and t->value to (char *) before freeing, which assumes these fields were allocated as char arrays. While this matches the original behavior, this pattern relies on implementation details of the memory allocator and could be problematic if the allocation types change. The casting suggests potential type safety issues in the memory management design. |
| [_mdns_create_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2706) | [create_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L332) | The refactored code introduces a potential resource leak in the error handling path of mdns_service_add_for_host. When create_service succeeds but the subsequent mdns_mem_malloc for the mdns_srv_item_t fails, the function calls free_service(s) but does not remove the service from the global s_server->services list. Additionally, the locking mechanism (mdns_priv_service_lock/unlock) wraps the entire function but the original _mdns_create_service was likely called within an already locked context, potentially creating deadlock risks or inconsistent locking behavior. |
| [_mdns_dealloc_scheduled_service_answers](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2762) | [dealloc_scheduled_service_answers](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1742) | The refactored function 'dealloc_scheduled_service_answers' has identical logic to the original but there's a potential issue with the second while loop. In the original function, when d->next is NULL but d itself matches the service, it won't be removed in the second loop. The refactored version preserves this same behavior, which might be intentional but could lead to leftover answers if the first element after the initial removal matches the service but has no next pointer. This appears to be a pre-existing issue carried over from the original implementation. |
| [_mdns_remove_scheduled_service_packets](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2787) | [mdns_priv_remove_scheduled_service_packets](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mock_mdns_send.c#L2171) | The refactored function passes the condition `had_answers && q->answers == NULL` to `mdns_priv_pcb_check_probing_services` for the announcing state check, but this condition is evaluated before the deallocation calls. In the original code, this condition was checked after deallocation, which could lead to different behavior if the deallocation functions modify the answers list during execution. This timing difference might cause incorrect PCB state transitions during service removal. |
| [_mdns_free_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2868) | [free_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L385) | The refactored function `free_subtype` appears to be a direct replacement for `_mdns_free_subtype` with identical logic, but there's a potential memory safety issue: the function casts `subtype->subtype` to `(char *)` before freeing, which suggests the original data might have been declared as `const char *`. This cast could mask const-correctness violations in the codebase. If the subtype string was originally allocated as read-only memory or through string literals, this could lead to undefined behavior when attempting to free it. |
| [_mdns_free_service_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2878) | [free_service_subtype](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L395) | |
| [_mdns_free_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2889) | [free_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L406) | The refactored function `free_service` calls `free_service_subtype(service)` which sets `service->subtype = NULL`, but this happens after the service structure has already been freed with `mdns_mem_free(service)`. This creates a use-after-free bug where we're writing to memory that has already been deallocated. In the original code, `_mdns_free_service_subtype` was called before freeing the service structure, which was correct. |
| [_mdns_check_srv_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2917) | [check_srv_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L438) | The refactored function hardcodes domain length as 5 bytes when copying MDNS_UTILS_DEFAULT_DOMAIN, but there's no verification that MDNS_UTILS_DEFAULT_DOMAIN is actually exactly 5 bytes. If the domain constant differs in length, this could cause buffer overruns or incorrect collision detection. Additionally, the function uses Variable Length Arrays (VLAs) which could cause stack overflow with very long hostnames, and the uint16_t index variables may overflow if the total data length exceeds 65535 bytes. |
| [_mdns_check_txt_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L2974) | [check_txt_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L393) | The refactored function uses mdns_priv_append_one_txt_record_entry instead of append_one_txt_record_entry. While both functions appear to have identical signatures and similar logic, the refactored version is no longer inline and has been moved to a separate compilation unit (mdns_send.c). This could potentially impact performance in the collision detection hot path, and there's a risk that the implementation might differ slightly in edge cases (like handling of empty values or special characters) which could affect the byte-for-byte comparison critical for proper mDNS collision resolution. |
| [mdns_pcb_deinit_local](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3016) | [deinit_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L107) | The NULL check on `pcb` in the refactored function is problematic. In the original code, `_pcb` was obtained via `&_mdns_server->interfaces[tcpip_if].pcbs[ip_proto]`, which would always return a valid address (not NULL) if the array structure exists. In the refactored code, `pcb = &s_pcbs[tcpip_if][ip_proto]` should also always return a valid address. This NULL check could mask actual initialization issues with the `s_pcbs` array and might prevent proper error propagation from `mdns_priv_if_deinit`. The check should either be removed or replaced with a proper array bounds validation. |
| [_mdns_dup_interface](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3035) | [mdns_priv_pcb_set_duplicate](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L186) | The refactored function uses a different data structure (s_pcbs[tcpip_if][i] vs _mdns_server->interfaces[tcpip_if].pcbs[i]) and the state transition to PCB_DUP is performed unconditionally without verifying if the PCB was previously in a valid state. This could lead to inconsistent state management if the PCB was already in PCB_OFF or another terminal state. Additionally, the function is no longer static, which changes its visibility and could affect encapsulation. |
| [_mdns_check_a_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3059) | [check_a_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L322) | The refactored function uses different helper functions (mdns_priv_get_esp_netif, mdns_priv_netif_get_other_interface, mdns_priv_pcb_set_duplicate) compared to the original (_mdns_get_esp_netif, _mdns_get_other_if, _mdns_dup_interface). While the function logic appears identical, there's a risk that these renamed helper functions might have different implementations or error handling behaviors that could affect the collision detection logic. Specifically, if mdns_priv_netif_get_other_interface returns different values than _mdns_get_other_if for edge cases, or if mdns_priv_pcb_set_duplicate has different side effects than _mdns_dup_interface, this could introduce subtle behavioral changes in the collision resolution protocol. |
| [_mdns_check_aaaa_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3095) | [check_aaaa_collision](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L358) | The refactored function uses renamed helper functions and constants whose implementations cannot be verified from the provided context. Specifically: <br/> 1. `mdns_priv_netif_get_other_interface` replaces `_mdns_get_other_if` - if this function has different behavior in edge cases (like when interfaces are disabled), it could affect collision detection logic <br/> 2. `mdns_priv_pcb_set_duplicate` replaces `_mdns_dup_interface` - if this function performs additional operations or misses some operations from the original, it could impact interface state management <br/> 3. `MDNS_UTILS_SIZEOF_IP6_ADDR` must exactly match the original `_MDNS_SIZEOF_IP6_ADDR` (typically 16 bytes for IPv6 addresses), otherwise memcmp comparisons will be incorrect <br/> <br/> The functional logic appears identical, but without verifying the implementations of these renamed components, there's risk of subtle behavioral changes. |
| [_hostname_is_ours](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3127) | [mdns_utils_hostname_is_ours](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L105) | The refactored function mdns_utils_hostname_is_ours accesses global data through mdns_priv_get_global_hostname() and mdns_priv_get_hosts() without any locking mechanism, while the original function was called within MDNS_SERVICE_LOCK/UNLOCK sections. This could lead to race conditions if the global hostname or host list is modified concurrently. The wrapper function mdns_hostname_exists does provide locking, but direct calls to mdns_utils_hostname_is_ours without locking could access inconsistent state. |
| [_mdns_delegate_hostname_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3150) | [mdns_priv_delegate_hostname_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L426) | The refactored function uses s_server->host_list instead of the global _mdns_host_list, but there's no null check for s_server. If s_server is NULL, this will cause a segmentation fault when accessing s_server->host_list. The original function directly used the global variable _mdns_host_list which would always be accessible (though potentially NULL for an empty list). This introduces a new failure mode that wasn't present in the original implementation. |
| [free_address_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3168) | [mdns_utils_free_address_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L239) | |
| [_mdns_delegate_hostname_set_address](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3178) | [delegate_hostname_set_address](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L444) | The refactored function delegate_hostname_set_address uses mdns_utils_free_address_list instead of the original free_address_list, but the memory ownership semantics may have changed. In the original code, when ACTION_DELEGATE_HOSTNAME_SET_ADDR action fails, the caller explicitly frees the address_list, suggesting the function takes ownership. The refactored version appears to directly assign the address_list to the host item without clear documentation of ownership transfer, which could lead to double-free or memory leaks if the memory management contract has changed. |
| [copy_address_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3198) | [mdns_utils_copy_address_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L214) | The refactored function introduces a HOOK_MALLOC_FAILED macro call before freeing the partially allocated list. If this hook has side effects (like logging, delays, or state changes), it could potentially affect error handling behavior or timing in memory-constrained scenarios. Additionally, the function now depends on mdns_utils_free_address_list being available and functionally equivalent to the original free_address_list. |
| [free_delegated_hostnames](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3222) | [free_delegated_hostnames](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L49) | The refactored function now accesses s_server->host_list instead of the global _mdns_host_list, but there is no NULL check for s_server. If free_delegated_hostnames is called when s_server is NULL (e.g., during shutdown or error conditions), this will cause a segmentation fault. Additionally, the function mdns_priv_responder_free calls free_delegated_hostnames after checking if s_server is NULL, suggesting that s_server could be NULL in some scenarios. |
| [_mdns_delegate_hostname_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3235) | [delegate_hostname_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L464) | The refactored function uses different global variables and function names without clear verification that they maintain identical behavior. Specifically: s_server vs _mdns_server, mdns_priv_pcb_send_bye_service vs _mdns_send_bye, mdns_priv_remove_scheduled_service_packets vs _mdns_remove_scheduled_service_packets, and free_service vs _mdns_free_service. The host list management also changed from _mdns_host_list to s_server->host_list. These changes could introduce subtle bugs if the underlying implementations differ, especially around memory management, protocol behavior, or state consistency. |
| [_mdns_name_is_discovery](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3282) | [is_discovery](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L28) | |
| [_mdns_name_is_selfhosted](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3296) | [is_name_selfhosted](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L495) | The refactored function `is_name_selfhosted` introduces a potential null pointer dereference. The original function accessed `_mdns_server->hostname` directly, while the refactored version calls `mdns_priv_get_global_hostname()` without checking if the returned pointer is valid before passing it to `mdns_utils_str_null_or_empty()`. If `mdns_priv_get_global_hostname()` can return NULL, this could lead to undefined behavior. |
| [_mdns_name_is_ours](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3319) | [is_ours](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L60) | The refactored function uses mdns_utils_hostname_is_ours which now checks against multiple hosts from mdns_priv_get_hosts() in addition to the global hostname. This expands the definition of "ours" to include additional hosts, which may break assumptions in the original code that only the global hostname was considered. Additionally, the change from _mdns_server->hostname to mdns_priv_get_global_hostname() could have different semantics if the underlying data structure or initialization timing has changed. |
| [_mdns_read_u16](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3385) | [mdns_utils_read_u16](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.h#L18) | |
| [_mdns_read_u32](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3397) | [mdns_utils_read_u32](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.h#L30) | |
| [_mdns_parse_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3411) | [mdns_utils_parse_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L71) | The refactoring changed the function scope from static to external (non-static), which could lead to unintended access from other compilation units and potential namespace pollution. Additionally, the static buffer usage in both mdns_utils_parse_fqdn and mdns_utils_read_fqdn creates reentrancy issues in multi-threaded environments or during recursive calls. |
| [_mdns_question_matches](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3448) | [question_matches](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L518) | The refactored function uses MDNS_UTILS_DEFAULT_DOMAIN instead of MDNS_DEFAULT_DOMAIN, which could introduce subtle bugs if these constants have different values or if MDNS_UTILS_DEFAULT_DOMAIN is not properly defined in the refactored codebase. Additionally, the function _mdns_get_service_instance_name was renamed to mdns_utils_get_service_instance_name, and any behavioral differences between these functions could affect the matching logic for SRV and TXT record types. |
| [_mdns_remove_parsed_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3483) | [remove_parsed_question](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L553) | |
| [_mdns_txt_items_count_get](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3515) | [get_txt_items_count](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L181) | |
| [_mdns_txt_item_name_get_len](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3542) | [get_txt_item_len](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L208) | The refactored code in result_txt_create removed the bounds check "txt_num >= num_items" that was present in the original code. In the original implementation, the condition was "if (name_len < 0 || txt_num >= num_items)" which provided protection against buffer overruns when the actual number of parsed TXT items exceeds the pre-allocated array size. The refactored version only checks "if (name_len < 0)" which could lead to writing beyond the allocated txt array bounds if get_txt_items_count returns an incorrect value or if the TXT data contains more valid items than expected. |
| [_mdns_result_txt_create](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3558) | [result_txt_create](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L224) | The refactored function removed the `txt_num >= num_items` check from the invalid item condition, relying solely on the while loop condition `txt_num < num_items`. This creates a potential buffer overrun vulnerability if there are many invalid TXT items (where name_len < 0) followed by valid ones. The original code had double protection by checking both conditions, but the refactored version only checks the array bounds in the loop condition, which could be bypassed if invalid items cause the loop to continue without incrementing txt_num properly. |
| [_mdns_strdup_check](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3654) | [strdup_check](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L122) | |
| [mdns_parse_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L3672) | [mdns_parse_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_receive.c#L587) | The refactored function has been changed from public to static scope, which could break external callers. Additionally, there are potential subtle bugs in the collision detection logic where the refactored version uses simplified PCB state checking via `mdns_priv_pcb_is_probing(packet)` instead of the original direct structure access `_mdns_server->interfaces[packet->tcpip_if].pcbs[packet->ip_protocol].probe_running`. This abstraction might not handle all edge cases identically, particularly during concurrent probe operations or interface state transitions. |
| [_mdns_enable_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4362) | [mdns_priv_pcb_enable](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L172) | |
| [_mdns_disable_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4376) | [mdns_priv_pcb_disable](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L153) | The refactored function changes the data structure access pattern from `_mdns_server->interfaces[tcpip_if].pcbs[ip_protocol].state` to `s_pcbs[tcpip_if][ip_protocol].state`, which represents a significant architectural change. This could introduce subtle bugs related to: <br/> 1. Memory layout differences affecting state synchronization <br/> 2. Race conditions if the new s_pcbs structure has different locking mechanisms <br/> 3. Potential null pointer dereferences if s_pcbs array initialization differs from the original _mdns_server structure <br/> 4. Consistency issues with the PCB state transitions across the codebase due to the structural change <br/> <br/> Additionally, the function renaming (`mdns_pcb_deinit_local` → `deinit_pcb`, `_mdns_get_other_if` → `mdns_priv_netif_get_other_interface`) suggests broader architectural changes that could affect error handling paths or edge case behavior. |
| [nibble_to_hex](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4393) | [nibble_to_hex](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L46) | |
| [perform_event_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4402) | [perform_event_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L55) | The refactored function maintains the same logic but introduces potential memory leaks in the reverse DNS lookup code. In the IPv4 reverse lookup, asprintf allocates memory for reverse_query_name but there's no free() call if mdns_priv_delegate_hostname_add succeeds. Similarly, in IPv6 reverse lookup, mdns_mem_malloc allocates memory but there's no corresponding free. The original code had the same issue, but this refactoring missed the opportunity to fix it. |
| [post_mdns_disable_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4473) | [post_disable_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L183) | |
| [post_mdns_enable_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4478) | [post_enable_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L189) | |
| [post_mdns_announce_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4483) | [post_announce_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L195) | |
| [mdns_preset_if_handle_system_event](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4489) | [handle_system_event_for_preset](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L202) | The refactored function replaces manual browse iteration with mdns_priv_browse_send_all(), but without seeing the implementation of this function, we cannot verify if it maintains the same iteration behavior as the original _mdns_browse_send() calls. Additionally, the function name changes (esp_netif_from_preset_if to netif_from_preset, etc.) suggest potential semantic differences that need verification. |
| [_mdns_search_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4582) | [search_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L343) | The refactored search_free function does not check if the search parameter is NULL before dereferencing its fields. While the original function also lacked this check, the refactoring moved the function to a different context (mdns_querier.c) where the calling patterns might have changed. In C programming, defensive programming practices suggest checking for NULL pointers before dereferencing, especially for cleanup functions that might be called from multiple contexts with potentially invalid parameters. |
| [_mdns_search_init](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4594) | [search_init](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L355) | |
| [_mdns_search_finish](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4652) | [search_finish](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L59) | The refactored function changes the global variable from `_mdns_server->search_once` to `s_search_once`, but there's no evidence that the ACTION_SEARCH_END case in the action handler was updated to call the new function. This could leave dangling searches that are never properly finished when triggered through the action system, potentially causing resource leaks or synchronization issues with the semaphore. |
| [_mdns_search_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4665) | [search_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L72) | |
| [_mdns_search_finish_done](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4674) | [mdns_priv_query_done](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L181) | The refactored function uses a global variable `s_search_once` instead of accessing it through `_mdns_server->search_once`, which changes the data structure access pattern. Additionally, the new `search_finish` function performs different cleanup operations (queue detachment and semaphore signaling) compared to the original `_mdns_search_finish` function, whose implementation is not shown for verification. This could lead to inconsistent search state management or resource cleanup issues. |
| [_mdns_result_addr_create_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4690) | [mdns_priv_result_addr_create_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L653) | The function visibility changed from static to non-static, which could lead to unintended usage across the codebase. Additionally, the refactored function is now called from new contexts (result_add_ip and mdns_priv_query_result_add_ip) that weren't in the original codebase, potentially introducing different error handling requirements. While the core implementation remains identical, the architectural changes in how IP addresses are managed could affect resource cleanup patterns in case of failures. |
| [_mdns_result_update_ttl](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4707) | [mdns_priv_query_update_result_ttl](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.h#L99) | |
| [_mdns_result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4715) | [result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L485) | The refactoring introduces a subtle bug in the memory management logic. In the original code, when _mdns_result_addr_create_ip fails, the function simply returns without affecting the result structure. However, in mdns_priv_query_result_add_ip, when mdns_priv_result_addr_create_ip fails during new result creation, the partially allocated mdns_result_t structure is freed, but the search->result pointer is never updated, leaving the search result list in an inconsistent state. Additionally, the hostname string duplication occurs after IP address creation, which could lead to memory leaks if the IP creation fails but the hostname was already allocated. |
| [_mdns_result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4715) | [mdns_priv_query_result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L514) | The refactoring introduces a subtle bug in the memory management logic. In the original code, when _mdns_result_addr_create_ip fails, the function simply returns without affecting the result structure. However, in mdns_priv_query_result_add_ip, when mdns_priv_result_addr_create_ip fails during new result creation, the partially allocated mdns_result_t structure is freed, but the search->result pointer is never updated, leaving the search result list in an inconsistent state. Additionally, the hostname string duplication occurs after IP address creation, which could lead to memory leaks if the IP creation fails but the hostname was already allocated. |
| [_mdns_search_result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4744) | [mdns_priv_query_result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L514) | The function 'result_add_ip' is called without the 'mdns_priv_' prefix, which may indicate either a missing namespace prefix or an undefined function. This could lead to linking errors or incorrect behavior if the intended function is not available. All other helper functions were properly prefixed with 'mdns_priv_' or 'mdns_utils_', making this inconsistency potentially problematic. |
| [_mdns_search_result_add_ptr](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4802) | [mdns_priv_query_result_add_ptr](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L614) | The refactored function mdns_priv_query_result_add_ptr appears to have a critical bug: callers in the refactored code (mdns_receive.c lines 851-852, 922-923, 1038-1040) do not check the return value for NULL, unlike the original code which explicitly checked for NULL returns (lines 4020-4023, 4132-4135). This could lead to NULL pointer dereferences if the function fails to allocate memory or encounters other errors. The function signature still returns mdns_result_t*, suggesting it should be checked, but the calling code ignores the return value. |
| [_mdns_search_result_add_srv](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4844) | [mdns_priv_query_result_add_srv](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L572) | |
| [_mdns_search_result_add_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4886) | [mdns_priv_query_result_add_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L435) | The refactoring splits the original unified TXT handling function into two specialized functions (query vs browse) with different signatures. The original parser call site at line 4149 now needs to determine which function to call based on context, but the additional parameters required by mdns_priv_browse_result_add_txt (instance, service, proto) may not be available in all calling contexts where the original function was used. This could lead to either incorrect function selection or missing data when calling the browse version, potentially causing incomplete TXT record processing or memory access issues. |
| [_mdns_search_find_from](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L4936) | [mdns_priv_query_find_from](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L197) | The function has been changed from static to non-static linkage, which could expose internal implementation details and create unintended dependencies. Additionally, the utility function replacements (_mdns_get_esp_netif → mdns_priv_get_esp_netif and _str_null_or_empty → mdns_utils_str_null_or_empty) need verification that they maintain identical behavior in all edge cases, particularly for NULL inputs and network interface mapping. |
| [_mdns_create_search_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5003) | [create_search_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L269) | The refactored function uses `mdns_priv_alloc_packet` instead of `_mdns_alloc_packet_default`. If `mdns_priv_alloc_packet` doesn't provide the same default packet initialization (like setting default flags, TTL values, or other packet metadata), it could result in malformed mDNS search packets that don't conform to protocol specifications. Additionally, the domain constant change from `MDNS_DEFAULT_DOMAIN` to `MDNS_UTILS_DEFAULT_DOMAIN` could affect DNS query formation if the values differ, potentially causing search queries to use the wrong domain. |
| [_mdns_search_send_pcb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5060) | [mdns_priv_query_send](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L327) | The refactored function uses `mdsn_priv_pcb_is_inited(tcpip_if, ip_protocol)` instead of the original complex condition `mdns_is_netif_ready(tcpip_if, ip_protocol) && _mdns_server->interfaces[tcpip_if].pcbs[ip_protocol].state > PCB_INIT`. This could introduce subtle bugs if the new function doesn't properly replicate both the network interface readiness check AND the PCB state validation. Additionally, there appears to be a potential typo in the function name "mdsn_priv_pcb_is_inited" (missing 'n' in 'mdns'). |
| [_mdns_search_send](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5076) | [search_send](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L81) | The refactored function `mdns_priv_query_send` (which replaces `_mdns_search_send_pcb`) immediately frees the packet after dispatch with `mdns_priv_free_tx_packet(packet)`, while the original implementation's packet lifecycle management is unclear. This could cause issues if packet transmission is asynchronous and the packet needs to remain valid until transmission completes. Additionally, the refactored version introduces early return on packet creation failure, which changes the error handling behavior compared to the original loop structure. |
| [_mdns_tx_handle_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5102) | [handle_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1816) | The refactoring splits the original function's logic across multiple functions, but introduces a potential bug in the PCB_PROBE_3 case. In the original code, when creating an announce packet fails, it breaks from the switch statement after scheduling a retry. However, in the refactored version, the variable 'p' is reassigned to 'a' (the new announce packet) in the success case, but this reassignment only affects the local copy in mdns_priv_pcb_schedule_tx_packet. The calling function handle_packet still holds the original 'p' pointer, which could lead to double-free or use-after-free issues if the packet lifecycle isn't properly managed across the function boundaries. |
| [_mdns_tx_handle_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5102) | [mdns_priv_pcb_schedule_tx_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_pcb.c#L211) | The refactoring splits the original function's logic across multiple functions, but introduces a potential bug in the PCB_PROBE_3 case. In the original code, when creating an announce packet fails, it breaks from the switch statement after scheduling a retry. However, in the refactored version, the variable 'p' is reassigned to 'a' (the new announce packet) in the success case, but this reassignment only affects the local copy in mdns_priv_pcb_schedule_tx_packet. The calling function handle_packet still holds the original 'p' pointer, which could lead to double-free or use-after-free issues if the packet lifecycle isn't properly managed across the function boundaries. |
| [_mdns_remap_self_service_hostname](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5159) | [mdns_priv_remap_self_service_hostname](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L508) | The function scope change from static to non-static could potentially cause linking conflicts if this function is now accessible from other compilation units. Additionally, the global variable change from _mdns_server to s_server requires verification that s_server is properly initialized and equivalent to the original _mdns_server in all execution contexts. |
| [_mdns_sync_browse_result_link_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5173) | [sync_browse_result_link_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L236) | The function name changed from `_mdns_sync_browse_result_link_free` to `sync_browse_result_link_free` (removing the leading underscore and "mdns_" prefix). While the core logic remains identical, this naming change could introduce subtle bugs if: <br/> 1. Call sites in the refactored codebase haven't been updated to use the new function name <br/> 2. The function visibility/scope changed due to the naming convention alteration <br/> 3. Other parts of the codebase rely on the specific naming pattern for internal functions <br/> <br/> The original function was called in ACTION_BROWSE_SYNC cases in mdns.c, and if those call sites weren't properly updated to reference the renamed function, it would result in compilation errors or runtime failures due to undefined function references. |
| [_mdns_free_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5188) | [free_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L126) | The refactored free_action function delegates cleanup to external functions (mdns_priv_*_action with ACTION_CLEANUP mode) without visible implementation. This introduces a risk that the specific memory cleanup operations performed in the original function (mdns_mem_free for hostnames, _mdns_search_free for searches, free_address_list for address lists) may not be properly handled, potentially leading to memory leaks. The architectural change from direct cleanup to delegation requires verification that all cleanup functions properly handle their respective resource types. |
| [_mdns_execute_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5235) | [execute_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_engine.c#L15) | The refactoring introduces several subtle bugs: <br/> <br/> 1. **Missing ACTION_SYSTEM_EVENT handling**: In Result 2 (mdns_engine.c), the ACTION_SYSTEM_EVENT case is completely empty, while the original function called perform_event_action(). This will break system event processing. <br/> <br/> 2. **Missing semaphore operations**: The original ACTION_HOSTNAME_SET case called xSemaphoreGive(_mdns_server->action_sema) after completion, but the refactored version delegates to mdns_priv_responder_action without preserving this critical synchronization. <br/> <br/> 3. **Missing memory cleanup in ACTION_BROWSE_SYNC**: The original function explicitly called _mdns_sync_browse_result_link_free() for ACTION_BROWSE_SYNC, but the refactored version delegates this to mdns_priv_browse_action without clear evidence this cleanup is preserved. <br/> <br/> 4. **Split responsibility confusion**: The introduction of both execute_action and free_action functions suggests the original single responsibility has been split, but it's unclear when each should be called. This could lead to double-free or memory leaks if not properly coordinated. <br/> <br/> 5. **Inconsistent implementations**: Having two different execute_action functions (Result 1 vs Result 2) with different behavior for ACTION_SYSTEM_EVENT suggests either incomplete refactoring or platform-specific variations that need verification. |
| [_mdns_execute_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5235) | [free_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L126) | The refactoring introduces several subtle bugs: <br/> <br/> 1. **Missing ACTION_SYSTEM_EVENT handling**: In Result 2 (mdns_engine.c), the ACTION_SYSTEM_EVENT case is completely empty, while the original function called perform_event_action(). This will break system event processing. <br/> <br/> 2. **Missing semaphore operations**: The original ACTION_HOSTNAME_SET case called xSemaphoreGive(_mdns_server->action_sema) after completion, but the refactored version delegates to mdns_priv_responder_action without preserving this critical synchronization. <br/> <br/> 3. **Missing memory cleanup in ACTION_BROWSE_SYNC**: The original function explicitly called _mdns_sync_browse_result_link_free() for ACTION_BROWSE_SYNC, but the refactored version delegates this to mdns_priv_browse_action without clear evidence this cleanup is preserved. <br/> <br/> 4. **Split responsibility confusion**: The introduction of both execute_action and free_action functions suggests the original single responsibility has been split, but it's unclear when each should be called. This could lead to double-free or memory leaks if not properly coordinated. <br/> <br/> 5. **Inconsistent implementations**: Having two different execute_action functions (Result 1 vs Result 2) with different behavior for ACTION_SYSTEM_EVENT suggests either incomplete refactoring or platform-specific variations that need verification. |
| [_mdns_send_search_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5322) | [send_search_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L413) | The refactored function replaces direct queue access `xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0)` with a wrapper function `mdns_priv_queue_action(action)`. Without seeing the implementation of `mdns_priv_queue_action`, we cannot verify if it maintains the same non-blocking behavior (0 timeout) and return value semantics. If `mdns_priv_queue_action` uses blocking behavior or has different error handling, it could introduce timing issues or resource management problems in the MDNS search functionality. |
| [_mdns_scheduler_run](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5348) | [mdns_priv_send_packets](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L1703) | The refactored function uses `mdns_priv_queue_action(action)` instead of directly calling `xQueueSend`. While this appears to be a wrapper function, there's a potential subtle bug: if `mdns_priv_queue_action` has different timeout behavior than the original `(TickType_t)0` (non-blocking), it could introduce blocking behavior that wasn't present in the original implementation. Additionally, the error handling logic assumes `mdns_priv_queue_action` returns false on failure (matching the original `xQueueSend != pdPASS`), but this needs verification to ensure the same cleanup behavior is maintained. |
| [_mdns_search_run](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5385) | [mdns_priv_query_start_stop](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L133) | The refactored function mdns_priv_query_start_stop introduces a subtle bug in error handling. In the original code, when _mdns_send_search_action(ACTION_SEARCH_END, s) fails, the search state is restored to SEARCH_RUNNING, allowing retry in the next iteration. However, in the refactored version, when send_search_action(ACTION_SEARCH_END, s) fails, the state is also restored to SEARCH_RUNNING, but the timeout condition (now > (s->started_at + s->timeout)) will remain true in subsequent iterations, causing the search to immediately try to end again in an infinite loop until the send succeeds. This could lead to excessive retries and potential resource exhaustion. |
| [_mdns_service_task](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5417) | [service_task](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L200) | The refactored function replaces the direct pointer check `_mdns_server && _mdns_server->action_queue` with a function call `mdns_priv_is_server_init()` and a global variable `s_action_queue`. This introduces potential issues: <br/> 1. The function call `mdns_priv_is_server_init()` may have different timing/side effects compared to the simple pointer check <br/> 2. The global `s_action_queue` is not protected by the same locking mechanism as the original server structure <br/> 3. There's a potential race condition if `s_action_queue` is modified while the service task is running <br/> 4. The separation of server initialization check from queue access could lead to inconsistent state detection |
| [_mdns_timer_cb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5439) | [timer_cb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L222) | The refactoring changes the core functionality of the timer callback from scheduler/search operations to packet sending/query operations. While the structural mapping is clear (timer_cb replaces _mdns_timer_cb and start_timer replaces _mdns_start_timer), the functional equivalence cannot be verified without understanding what mdns_priv_send_packets and mdns_priv_query_start_stop do compared to the original _mdns_scheduler_run and _mdns_search_run. This could introduce subtle bugs in MDNS protocol timing, packet scheduling, or service discovery behavior. |
| [_mdns_timer_cb](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5439) | [start_timer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L228) | The refactoring changes the core functionality of the timer callback from scheduler/search operations to packet sending/query operations. While the structural mapping is clear (timer_cb replaces _mdns_timer_cb and start_timer replaces _mdns_start_timer), the functional equivalence cannot be verified without understanding what mdns_priv_send_packets and mdns_priv_query_start_stop do compared to the original _mdns_scheduler_run and _mdns_search_run. This could introduce subtle bugs in MDNS protocol timing, packet scheduling, or service discovery behavior. |
| [_mdns_start_timer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5445) | [start_timer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L228) | The refactored start_timer function uses a global variable s_timer_handle instead of the original _mdns_server->timer_handle, but there's no evidence that s_timer_handle is properly initialized to NULL. This could lead to double-creation of timers if the function is called multiple times, as the original code would have stored the handle in a server structure that gets reinitialized. Additionally, the callback function was renamed from _mdns_timer_cb to timer_cb without verification that the implementation remains equivalent. |
| [_mdns_stop_timer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5460) | [stop_timer](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L243) | |
| [_mdns_task_create_with_caps](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5473) | [create_task_with_caps](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L256) | The refactored function introduces a new error handling path with 'alloc_failed' label that calls HOOK_MALLOC_FAILED before proceeding to the 'err' label. This changes the control flow and could potentially cause issues if HOOK_MALLOC_FAILED has side effects or if the order of cleanup operations matters. In the original function, memory allocation failure went directly to the cleanup code, but now there's an intermediate step that might interfere with proper resource management. |
| [_mdns_service_task_start](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5498) | [service_task_start](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L283) | |
| [_mdns_service_task_stop](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5537) | [service_task_stop](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L320) | The refactored function maintains the same potential resource management issue as the original: if xQueueSend fails, the semaphore gets deleted immediately while the task might still be running and accessing it. However, there's an additional subtle change - the refactored version uses different timer functions (esp_timer_stop/delete vs original _mdns_stop_timer) which could have different error handling behavior. The original _mdns_stop_timer might have handled timer state differently than the new stop_timer function. |
| [mdns_post_custom_action_tcpip_if](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5558) | [post_custom_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L159) | The refactored function inverts the queue operation logic: original uses `xQueueSend(...) != pdPASS` for failure case, while refactored uses `!mdns_priv_queue_action(action)` for failure case. This suggests `mdns_priv_queue_action` returns true on success, but without seeing its implementation, we cannot verify if the queue behavior is equivalent. Additionally, the server initialization check changed from direct `!_mdns_server` to `!mdns_priv_is_server_init()` which may have different semantics. |
| [set_default_duplicated_interfaces](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5579) | [set_default_duplicated_interfaces](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L285) | The function appears to be identical in both implementations (100% similarity), but there's a potential architectural concern. The refactored codebase contains mdns_priv_pcb_set_duplicate which handles PCB-level duplication with additional operations (clearing TX queues, deinitializing PCBs, setting PCB_DUP state), while the original netif-level duplication logic remains unchanged. This creates a potential inconsistency where netif-level duplication is established but PCB-level duplication handling may not be properly synchronized. The bidirectional relationship between interfaces is managed differently in these two approaches, which could lead to race conditions or inconsistent state between netif and PCB layers. |
| [unregister_predefined_handlers](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5597) | [mdns_priv_netif_unregister_predefined_handlers](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L303) | The refactored function uses a different event handler function name: 'handle_system_event_for_preset' instead of 'mdns_preset_if_handle_system_event'. While the conditional compilation logic is identical, this change could introduce subtle bugs if: <br/> 1. The new handler function has different behavior or implementation <br/> 2. The new handler function has a different signature that's not compatible with the event system <br/> 3. The original handler function 'mdns_preset_if_handle_system_event' is still registered elsewhere and not properly cleaned up <br/> 4. The new handler function doesn't exist or has different error handling <br/> <br/> The search shows 'mdns_preset_if_handle_system_event' exists in the original codebase with a specific implementation, but no matches were found for 'handle_system_event_for_preset' in the refactored codebase, suggesting it might be missing or renamed differently. |
| [mdns_netif_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5614) | [mdns_netif_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L384) | The refactored post_custom_action function returns ESP_OK even when mdns_priv_queue_action fails (returns false), which occurs when memory allocation for the action fails. This masks memory allocation failures and changes the error behavior from the original implementation. The original mdns_post_custom_action_tcpip_if likely returned ESP_ERR_NO_MEM in this scenario, but the refactored version returns success, potentially hiding critical resource constraints. |
| [mdns_register_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5619) | [mdns_register_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L389) | The refactored mdns_unregister_netif function in Result 2 has a critical bug where it calls mdns_priv_service_lock() twice at the end instead of unlocking. This will cause a deadlock situation where the service lock is never released, blocking all subsequent mDNS operations that require the lock. The correct pattern should be mdns_priv_service_unlock() before returning. |
| [mdns_unregister_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5645) | [mdns_unregister_netif](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_netif.c#L415) | The refactored mdns_unregister_netif function has a critical bug where it calls mdns_priv_service_lock() twice instead of calling mdns_priv_service_unlock() to release the lock. This will cause a deadlock situation as the service lock is never released, preventing other threads from accessing the shared resource and potentially hanging the system. |
| [mdns_init](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5665) | [mdns_init](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L351) | The refactored mdns_init function has a subtle bug in error handling: when mdns_priv_netif_init() fails, the code jumps to free_queue label which only deletes the action queue, but does not delete the semaphore created in mdns_priv_responder_init(). In the original code, both the queue and semaphore were properly cleaned up in the error path. This could lead to a resource leak of the semaphore. |
| [mdns_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5763) | [mdns_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_service.c#L391) | The refactored mdns_free function introduces a subtle bug in the cleanup sequence. In the original code, free_delegated_hostnames() was called before mdns_mem_free((char *)_mdns_server->hostname) and mdns_mem_free((char *)_mdns_server->instance). However, in the refactored version, free_delegated_hostnames() is called inside mdns_priv_responder_free() AFTER the hostname and instance have already been freed. This could lead to accessing freed memory if free_delegated_hostnames() references the server's hostname or instance fields. |
| [mdns_hostname_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5816) | [mdns_hostname_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L591) | The refactored function replaces direct xQueueSend with mdns_priv_queue_action, but there's a potential synchronization bug. The original code used xQueueSend with 0 timeout (non-blocking) and returned ESP_ERR_NO_MEM on queue full. The new mdns_priv_queue_action function's implementation is unknown - if it uses a different timeout or blocking behavior, it could cause deadlocks or different memory error handling. Additionally, the semaphore take operation (xSemaphoreTake) remains but the queue mechanism changed, which could lead to inconsistent state if the new queue function doesn't properly coordinate with the semaphore signaling. |
| [mdns_hostname_get](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5846) | [mdns_hostname_get](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L621) | |
| [mdns_delegate_hostname_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5864) | [mdns_delegate_hostname_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L639) | The refactored function uses mdns_priv_queue_action() instead of xQueueSend() with (TickType_t)0 timeout. If mdns_priv_queue_action() has different queue semantics (e.g., different timeout behavior or error handling), it could introduce subtle bugs in memory management or action queuing reliability. Additionally, the replacement of copy_address_list() with mdns_utils_copy_address_list() requires verification that the memory allocation and cleanup behavior remains identical, especially in error scenarios. |
| [mdns_delegate_hostname_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5895) | [mdns_delegate_hostname_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L670) | The refactored function shows inconsistent synchronization behavior compared to similar functions in the same codebase. While mdns_hostname_set and mdns_delegate_hostname_add both call xSemaphoreTake(s_server->action_sema, portMAX_DELAY) after queueing their actions, mdns_delegate_hostname_remove does not. This inconsistency could lead to race conditions where callers of mdns_delegate_hostname_remove assume the operation is complete when it may still be pending in the action queue. The original codebase also lacked this synchronization, but the refactoring introduced this pattern inconsistently across similar functions. |
| [mdns_delegate_hostname_set_address](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5924) | [mdns_delegate_hostname_set_address](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L699) | The refactored function removed the semaphore synchronization (xSemaphoreTake) that was present in similar functions (mdns_delegate_hostname_add and mdns_hostname_set), while the queue mechanism changed from direct xQueueSend to mdns_priv_queue_action. This creates inconsistent synchronization behavior across similar API functions and could lead to race conditions if the action handler expects synchronous completion. The original implementation used immediate queue sending (timeout 0) without waiting, but the refactored similar functions now wait for semaphore, suggesting this function may have missed a required synchronization update. |
| [mdns_hostname_exists](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5954) | [mdns_hostname_exists](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L729) | |
| [mdns_instance_name_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5963) | [mdns_instance_name_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L738) | The refactored function replaces direct xQueueSend with mdns_priv_queue_action, but the timeout behavior may have changed. The original used (TickType_t)0 timeout (non-blocking), while the helper function's timeout semantics are unclear. Additionally, the refactored version lacks the explicit semaphore synchronization present in mdns_hostname_set (xSemaphoreTake), potentially creating inconsistent behavior between similar operations. |
| [mdns_service_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L5996) | [mdns_service_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L767) | The refactored function has a critical bug in the linked list insertion logic. The original code correctly sets up the linked list by assigning the new item to the head of the services list. However, the refactored code has a logical error: it sets `item->next = NULL` and then immediately overwrites it with `item->next = s_server->services`, which is correct, but then assigns `s_server->services = item`. This appears correct at first glance, but there's a subtle issue - the original code properly maintains the linked list structure while the refactored version might have different behavior if the helper functions (like `mdns_utils_get_service_item_instance` and `create_service`) have different implementations than their original counterparts. Additionally, the memory cleanup function changed from `_mdns_free_service` to `free_service` - if these have different implementations, it could lead to memory leaks or corruption in error paths. |
| [mdns_service_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6038) | [mdns_service_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L809) | The refactored mdns_service_add function appears functionally equivalent to the original, but there are potential issues in its dependency chain. The function itself only changed the global variable name from _mdns_server to s_server, but the mdns_service_add_for_host function it calls has undergone significant refactoring with new locking mechanisms, service limit checks, and different error handling patterns. This could introduce subtle bugs like: <br/> 1. Deadlocks if the new mdns_priv_service_lock/unlock functions are not properly balanced <br/> 2. Different service validation behavior through mdns_utils_get_service_item_instance <br/> 3. Changed memory allocation patterns with create_service and HOOK_MALLOC_FAILED <br/> 4. Potential race conditions if the locking scope differs from the original implementation |
| [mdns_service_exists](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6047) | [mdns_service_exists](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L818) | |
| [mdns_service_exists_with_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6056) | [mdns_service_exists_with_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L827) | |
| [_copy_mdns_txt_items](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6066) | [copy_txt_items](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L837) | The error handling cleanup loop condition `y < ret_index + 1` in both original and refactored code appears to have a potential off-by-one error. When `ret_index` represents the number of successfully allocated items, accessing `ret[ret_index]` (which would be the `ret_index + 1`-th element) could read/write beyond the allocated memory bounds if an allocation failure occurs early in the second loop. The condition should likely be `y < ret_index` to only clean up the items that were actually allocated. |
| [_copy_delegated_host_address_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6119) | [copy_delegated_host_address_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L890) | |
| [_mdns_lookup_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6131) | [lookup_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_console.c#L1139) | |
| [mdns_service_port_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6211) | [mdns_service_port_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L982) | |
| [mdns_service_port_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6229) | [mdns_service_port_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1000) | |
| [mdns_service_txt_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6237) | [mdns_service_txt_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1008) | The refactored function has the same subtle bug as the original: when memory allocation fails (allocate_txt returns NULL), the function returns ESP_ERR_NO_MEM directly without releasing the service lock (mdns_priv_service_unlock). This creates a potential deadlock scenario where the service lock remains held indefinitely after a memory allocation failure. |
| [mdns_service_txt_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6267) | [mdns_service_txt_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1038) | The refactored function uses s_server instead of _mdns_server for the server state check. While both appear to serve the same purpose (global MDNS server state), the refactored code introduces a potential NULL pointer dereference in mdns_priv_responder_free() at line 64 where it accesses s_server->action_sema without first checking if s_server is NULL, unlike the original code which consistently checked _mdns_server before accessing its members. |
| [mdns_service_txt_item_set_for_host_with_explicit_value_len](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6275) | [mdns_service_txt_item_set_for_host_with_explicit_value_len](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1046) | The refactored function uses renamed helper functions (mdns_utils_str_null_or_empty, mdns_utils_get_service_item_instance, announce_all_pcbs, mdns_priv_service_lock/unlock) and a renamed global variable (s_server). While the core logic appears identical, there's a risk that these renamed functions may have different implementations or behaviors compared to their original counterparts. Specifically, the memory allocation error handling in the out_of_mem label still uses HOOK_MALLOC_FAILED and mdns_mem_free, but if the renamed locking functions don't properly handle error states or if the global s_server initialization differs from _mdns_server, it could lead to subtle bugs like memory leaks, race conditions, or incorrect service discovery behavior. |
| [mdns_service_txt_item_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6331) | [mdns_service_txt_item_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1102) | The refactored function appears identical to the original, but there are two related functions (mdns_service_txt_item_set and mdns_service_txt_item_set_with_explicit_value_len) that now include state checking for s_server, while the original mdns_service_txt_item_set_for_host does not. This creates an inconsistency where some TXT item setting functions check server state while others don't. Additionally, the global variable has been renamed from _mdns_server to s_server, which could indicate broader changes in the codebase that might affect error handling consistency. |
| [mdns_service_txt_item_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6339) | [mdns_service_txt_item_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1110) | |
| [mdns_service_txt_item_set_with_explicit_value_len](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6348) | [mdns_service_txt_item_set_with_explicit_value_len](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1119) | |
| [mdns_service_txt_item_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6357) | [mdns_service_txt_item_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1128) | The refactored function calls `announce_all_pcbs(&s, 1, false)` but there's no evidence that `announce_all_pcbs` exists in the refactored codebase. The search for "announce_all_pcbs" returned no matches, suggesting this function may be missing or renamed differently than expected. This could lead to a compilation error or runtime failure when trying to announce the TXT item removal. |
| [mdns_service_txt_item_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6404) | [mdns_service_txt_item_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1175) | The refactored mdns_service_txt_item_remove_for_host function has several potential issues: <br/> 1. The complex ESP_GOTO_ON_FALSE conditional combines multiple error checks (server state, service/proto/key validation) into a single error path, which may mask specific error conditions that were handled separately in the original implementation <br/> 2. The memory management uses mdns_mem_free() instead of the original free() calls, which could indicate inconsistent memory management strategy across the codebase <br/> 3. The announce_all_pcbs(&s, 1, false) call appears to be new functionality that wasn't present in the original TXT removal logic, potentially causing unnecessary network announcements <br/> 4. The error handling with HOOK_MALLOC_FAILED at the end seems misplaced since no memory allocation occurs in the main logic path of this function <br/> 5. The function returns ESP_OK even when no TXT item was found to remove (the function completes without setting an error when the key isn't found) |
| [_mdns_service_subtype_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6412) | [service_subtype_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1183) | The refactored mdns_service_subtype_remove_for_host function has a potential memory leak in the error path. When memory allocation fails at remove_subtypes->subtype = mdns_mem_strdup(subtype), the code jumps to out_of_mem label which frees remove_subtypes but does not free the already allocated remove_subtypes structure itself. This creates a scenario where remove_subtypes is allocated but its subtype field allocation fails, leaving the remove_subtypes structure leaked. The original function had no such memory allocation, so this is a new bug introduced during refactoring. |
| [_mdns_service_subtype_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6412) | [mdns_service_subtype_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1212) | The refactored mdns_service_subtype_remove_for_host function has a potential memory leak in the error path. When memory allocation fails at remove_subtypes->subtype = mdns_mem_strdup(subtype), the code jumps to out_of_mem label which frees remove_subtypes but does not free the already allocated remove_subtypes structure itself. This creates a scenario where remove_subtypes is allocated but its subtype field allocation fails, leaving the remove_subtypes structure leaked. The original function had no such memory allocation, so this is a new bug introduced during refactoring. |
| [mdns_service_subtype_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6441) | [mdns_service_subtype_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1212) | The refactored function uses `free_subtype(remove_subtypes)` instead of `_mdns_free_subtype(remove_subtypes)`. If `free_subtype` doesn't properly handle the case where `remove_subtypes->subtype` is NULL (due to failed mdns_mem_strdup), this could lead to memory leaks or undefined behavior. Additionally, the refactored version calls `free_subtype` even when `mdns_priv_send_bye_subtype` might have taken ownership of the subtypes, potentially causing double-free issues if the ownership semantics changed. |
| [_mdns_service_subtype_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6474) | [service_subtype_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1245) | The refactored function 'mdns_service_subtype_add_multiple_items_for_host' has inconsistent error handling compared to the original. In the original code, when _mdns_service_subtype_add_for_host returns ESP_ERR_INVALID_ARG (subtype already exists), the calling code continues processing other subtypes. However, in the refactored version, any error other than ESP_OK or ESP_ERR_NO_MEM causes immediate termination with goto exit, preventing processing of remaining subtypes and potentially leaving the service in an inconsistent state. Additionally, the memory cleanup rollback only occurs for ESP_ERR_NO_MEM cases, not for other error types. |
| [mdns_service_subtype_add_multiple_items_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6498) | [mdns_service_subtype_add_multiple_items_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1269) | The refactored function in Result 1 appears to be the direct replacement, but there are several subtle concerns: <br/> <br/> 1. **Missing validation for num_items > 0**: In the original code, the ESP_GOTO_ON_FALSE check includes `(num_items > 0)`, but in Result 1, this validation appears to be missing from the condition, which could allow calling the function with num_items=0 and bypass proper error handling. <br/> <br/> 2. **Function signature inconsistency**: The refactored function uses parameter name 'service' while the original uses 'service_type' in the header declaration. This could cause compilation issues if the header wasn't updated consistently. <br/> <br/> 3. **Resource management in error path**: The cleanup loop in the error path uses `service_subtype_remove_for_host` which may have different memory management behavior than the original `_mdns_service_subtype_remove_for_host`. If the memory allocation strategy changed (as seen in Result 3), the removal function might not properly handle the new allocation pattern. <br/> <br/> 4. **Potential double-free in update function**: Result 2 shows `free_subtype(goodbye_subtype)` followed by `free_service_subtype(s->service)` - if these functions have overlapping cleanup responsibilities, this could lead to double-free issues. |
| [mdns_service_subtype_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6535) | [mdns_service_subtype_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1306) | The refactoring introduced a subtle bug in the error handling logic. In the original implementation, when _mdns_service_subtype_add_for_host returns ESP_ERR_NO_MEM, the calling function mdns_service_subtype_add_multiple_items_for_host would continue processing other subtypes but eventually roll back all successful additions. However, in the refactored version, service_subtype_add_for_host immediately returns ESP_ERR_NO_MEM without proper cleanup of the allocated subtype_item, causing a memory leak. Additionally, the refactored version uses different error labels (err vs out_of_mem) and the memory cleanup path in service_subtype_add_for_host returns the same error code regardless of which allocation failed, losing error specificity. |
| [mdns_service_subtype_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6535) | [service_subtype_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1245) | The refactoring introduced a subtle bug in the error handling logic. In the original implementation, when _mdns_service_subtype_add_for_host returns ESP_ERR_NO_MEM, the calling function mdns_service_subtype_add_multiple_items_for_host would continue processing other subtypes but eventually roll back all successful additions. However, in the refactored version, service_subtype_add_for_host immediately returns ESP_ERR_NO_MEM without proper cleanup of the allocated subtype_item, causing a memory leak. Additionally, the refactored version uses different error labels (err vs out_of_mem) and the memory cleanup path in service_subtype_add_for_host returns the same error code regardless of which allocation failed, losing error specificity. |
| [mdns_service_subtype_add_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6535) | [mdns_service_subtype_add_multiple_items_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1269) | The refactoring introduced a subtle bug in the error handling logic. In the original implementation, when _mdns_service_subtype_add_for_host returns ESP_ERR_NO_MEM, the calling function mdns_service_subtype_add_multiple_items_for_host would continue processing other subtypes but eventually roll back all successful additions. However, in the refactored version, service_subtype_add_for_host immediately returns ESP_ERR_NO_MEM without proper cleanup of the allocated subtype_item, causing a memory leak. Additionally, the refactored version uses different error labels (err vs out_of_mem) and the memory cleanup path in service_subtype_add_for_host returns the same error code regardless of which allocation failed, losing error specificity. |
| [_mdns_service_find_subtype_needed_sendbye](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6543) | [service_find_subtype_needed_sendbye](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1314) | |
| [mdns_service_subtype_update_multiple_items_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6593) | [mdns_service_subtype_update_multiple_items_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1364) | The refactored function calls free_service_subtype(s->service) which completely clears all subtypes from the service before adding new ones in the loop. This differs from the original behavior where _mdns_free_service_subtype might have been more selective. This could cause temporary service unavailability and potential race conditions where subtypes are briefly unavailable between the free and add operations. Additionally, the refactored version uses different locking functions (mdns_priv_service_lock/unlock vs MDNS_SERVICE_LOCK/UNLOCK) which may have different thread safety guarantees. |
| [mdns_service_instance_name_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6640) | [mdns_service_instance_name_set_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1411) | |
| [mdns_service_instance_name_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6666) | [mdns_service_instance_name_set](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1437) | The refactored function in Result 1 appears to be functionally equivalent to the original, but there's a subtle variable name change from `_mdns_server` to `s_server`. While this is likely just a naming convention change, it could indicate broader architectural changes that might affect other parts of the codebase if the global server variable semantics have changed. The function maintains the same parameter signature and delegation pattern to `mdns_service_instance_name_set_for_host`. <br/> <br/> However, the presence of `mdns_instance_name_set` (Result 2) is concerning as it suggests a parallel API with different semantics - it only takes an instance parameter and implements the functionality directly rather than delegating. This could lead to confusion about which function to use and potential inconsistencies in service instance management. |
| [mdns_service_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6674) | [mdns_service_remove_for_host](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1445) | The refactored function calls `free_service(a->service)` and `mdns_mem_free(a)` after removing the service from the linked list, but the original function used `_mdns_free_service(a->service)`. Without seeing the implementation of `free_service` vs `_mdns_free_service`, there's a risk that the refactored version may have different memory management behavior, potentially leading to memory leaks or double-free issues if the implementations differ. |
| [mdns_service_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6727) | [mdns_service_remove](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1498) | |
| [mdns_service_remove_all](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6735) | [mdns_service_remove_all](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1506) | |
| [mdns_query_results_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6763) | [mdns_query_results_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L673) | |
| [_mdns_query_results_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6770) | [mdns_priv_query_results_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L25) | The refactoring changes the locking mechanism from MDNS_SERVICE_LOCK/UNLOCK to mdns_priv_service_lock/unlock without clear evidence of functional equivalence. This could introduce subtle synchronization issues, deadlock risks, or race conditions if the new locking functions have different semantics, scope, or implementation than the original ones. The change from static to public function scope (mdns_priv_query_results_free) also increases the risk of improper direct usage bypassing necessary locking. |
| [_mdns_query_results_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6770) | [mdns_query_results_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L673) | The refactoring changes the locking mechanism from MDNS_SERVICE_LOCK/UNLOCK to mdns_priv_service_lock/unlock without clear evidence of functional equivalence. This could introduce subtle synchronization issues, deadlock risks, or race conditions if the new locking functions have different semantics, scope, or implementation than the original ones. The change from static to public function scope (mdns_priv_query_results_free) also increases the risk of improper direct usage bypassing necessary locking. |
| [mdns_query_async_delete](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6801) | [mdns_query_async_delete](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L680) | |
| [mdns_query_async_get_results](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6817) | [mdns_query_async_get_results](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L696) | The function implementation remains identical, but the relocation from mdns.c to mdns_querier.c introduces potential architectural risks. The main concerns are: 1) Header file dependencies - mdns.h must properly declare the function and include necessary types from the new module, 2) Structure definition consistency - mdns_search_once_t must have identical layout in both compilation units, particularly the done_semaphore and result fields, 3) Build system integration - mdns_querier.c must be properly compiled and linked, and 4) Cross-module access - other functions in mdns.c that previously accessed this function directly may now require proper inter-module function calls. |
| [mdns_query_async_new](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6831) | [mdns_query_async_new](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L710) | The refactored search_init function now uses uint8_t for max_results parameter instead of size_t, which could cause truncation issues if max_results values exceed 255. This is a potential data loss bug since the original function accepted size_t and could handle larger values. Additionally, the search_init function creates a semaphore that needs verification of proper cleanup in all error paths through search_free. |
| [mdns_query_generic](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6853) | [mdns_query_generic](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L732) | The refactored function uses renamed helper functions (mdns_priv_is_server_init, mdns_utils_str_null_or_empty, search_init, send_search_action, search_free) without clear verification that these maintain the exact same behavior and error handling semantics as their original counterparts. Specifically: <br/> <br/> 1. The condition `mdns_utils_str_null_or_empty(service) != mdns_utils_str_null_or_empty(proto)` relies on the renamed string function returning consistent boolean values <br/> 2. The search initialization and action functions may have different memory management or error propagation behaviors <br/> 3. The semaphore wait on `search->done_semaphore` assumes the internal search structure and synchronization mechanism remains unchanged <br/> 4. Without seeing the implementations of these renamed functions, we cannot verify that resource cleanup (search_free) properly handles the results pointer assignment that occurs before freeing <br/> <br/> These renamed functions could introduce subtle behavioral changes in error cases, memory management, or protocol compliance. |
| [mdns_query](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6884) | [mdns_query](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L764) | The refactored mdns_query function appears identical to the original, but its dependency on mdns_query_generic has been significantly changed, introducing potential subtle bugs: <br/> <br/> 1. **Timeout handling regression**: The refactored mdns_query_generic uses `portMAX_DELAY` instead of the original timeout parameter, making queries potentially block indefinitely rather than respecting the user-specified timeout. <br/> <br/> 2. **Error code inconsistency**: The refactored version returns `ESP_ERR_NO_MEM` when `send_search_action` fails, but this function could fail for other reasons (network errors, internal state issues) that should return different error codes. <br/> <br/> 3. **Parameter validation change**: The validation logic for service/proto parameters has been altered from the original implementation, potentially allowing invalid parameter combinations that were previously rejected. <br/> <br/> 4. **Function dependency changes**: The refactored code uses `search_init`, `send_search_action`, and `search_free` instead of the original `_mdns_search_init` and internal mechanisms, raising concerns about functional equivalence and proper resource management. <br/> <br/> While the mdns_query wrapper function itself is unchanged, these underlying changes in mdns_query_generic could lead to unexpected blocking behavior, incorrect error reporting, and different parameter validation semantics. |
| [mdns_query_ptr](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6889) | [mdns_query_ptr](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L769) | |
| [mdns_query_srv](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6898) | [mdns_query_srv](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L778) | |
| [mdns_query_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6907) | [mdns_query_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L787) | |
| [mdns_lookup_delegated_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6916) | [mdns_lookup_delegated_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1531) | The refactored lookup_service function in mdns_console.c (Result 2) creates a circular dependency: it calls mdns_lookup_delegated_service/mdns_lookup_selfhosted_service, which in turn call the lookup_service function in mdns_responder.c. This appears to be a naming collision where two different functions with the same name exist in different compilation units, which could lead to linker errors or incorrect function resolution depending on the build configuration. |
| [mdns_lookup_selfhosted_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6931) | [mdns_lookup_selfhosted_service](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_responder.c#L1546) | The refactored code introduces a potential circular dependency. The function `lookup_service` in mdns_console.c calls `mdns_lookup_selfhosted_service`, which in turn calls `lookup_service` (the internal implementation). This creates infinite recursion. The original code called `_mdns_lookup_service` directly without this circular call pattern. Additionally, the parameter order in the `lookup_service` wrapper function appears inconsistent - it takes a `delegated` parameter but the actual implementation functions don't have this parameter in their signatures. |
| [mdns_query_a](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6947) | [mdns_query_a](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L797) | The refactored function changes the validation function from `_str_null_or_empty` to `mdns_utils_str_null_or_empty`. While this appears to be a simple renaming for better organization, there's a risk that the new function might have different behavior (e.g., different empty string definition, different return values, or different error handling) that could affect the input validation logic. The original codebase search didn't find references to `mdns_utils_str_null_or_empty`, suggesting this might be a newly introduced utility function whose implementation should be verified for functional equivalence. |
| [mdns_query_aaaa](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L6986) | [mdns_query_aaaa](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_querier.c#L836) | |
| [mdns_debug_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7026) | [dbg_packet](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_debug.c#L106) | The refactored function changes the timestamp type from uint32_t to uint64_t but uses the same format string logic. In embedded systems with 32-bit architectures, this could cause issues with printf formatting and stack alignment. Additionally, the static mdns_name_t variable could cause thread-safety issues in multi-threaded environments, and the new mdns_dbg_flush() call at the end might introduce blocking behavior that wasn't present in the original function. |
| [_mdns_sync_browse_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7256) | [mdns_priv_browse_sync](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L595) | The refactored function `mdns_priv_browse_sync` hardcodes the action type to `ACTION_BROWSE_SYNC`, removing the `type` parameter from the original function. This could be problematic if the original function was ever called with action types other than `ACTION_BROWSE_SYNC`. While the search shows only `ACTION_BROWSE_SYNC` usage in the original codebase, this change reduces flexibility and could break future extensions that might use different browse sync action types. |
| [_mdns_send_browse_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7278) | [send_browse_action](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L26) | The refactored function uses `!mdns_priv_queue_action(action)` instead of the original `xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS`. This boolean logic inversion suggests that `mdns_priv_queue_action` returns true for success and false for failure, while `xQueueSend` returns `pdPASS` (typically 1) for success and other values for failure. If `mdns_priv_queue_action` doesn't properly replicate the exact queueing semantics (including the zero timeout behavior), this could introduce subtle behavioral changes in the action queueing mechanism, potentially affecting the MDNS browsing functionality under high load conditions. |
| [_mdns_browse_item_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7301) | [browse_item_free](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L49) | |
| [_mdns_browse_init](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7314) | [browse_init](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L141) | The refactored browse_init function has inconsistent error handling for memory allocation failures. When browse->service allocation fails, HOOK_MALLOC_FAILED is called twice (once for browse malloc and once for service strndup), but when browse->proto allocation fails, HOOK_MALLOC_FAILED is only called once (for browse malloc). This inconsistency could lead to incorrect memory failure tracking or logging. The original function had consistent behavior where HOOK_MALLOC_FAILED was only called for the initial browse allocation failure. |
| [mdns_browse_new](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7345) | [mdns_browse_new](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L617) | The refactored mdns_browse_new function has a critical logic error in the server initialization check. The original code returned NULL when `!_mdns_server` (server not initialized), but the refactored code returns NULL when `mdns_priv_is_server_init()` (server IS initialized). This logical inversion would prevent browsing from working when the MDNS server is properly initialized, making the function always return NULL in normal operation. |
| [mdns_browse_delete](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7366) | [mdns_browse_delete](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L638) | |
| [_mdns_browse_finish](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7389) | [browse_finish](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L119) | The refactored function uses a global variable 's_browse' instead of accessing '_mdns_server->browse'. This structural change could introduce subtle bugs if: <br/> 1. 's_browse' is not properly synchronized with the server state in other parts of the system <br/> 2. There are concurrent access issues since global variables are more prone to race conditions <br/> 3. Other functions that previously operated on '_mdns_server->browse' might not be updated to use 's_browse' consistently <br/> 4. The initialization and lifetime management of 's_browse' might differ from the server-based approach |
| [_mdns_browse_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7411) | [browse_add](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L176) | The refactored function uses a global variable 's_browse' instead of accessing it through '_mdns_server->browse'. This structural change could introduce subtle bugs if: <br/> 1. 's_browse' is not properly initialized or synchronized with the original server state <br/> 2. Other parts of the system still expect to access browse data through '_mdns_server->browse' <br/> 3. There are thread safety issues since global 's_browse' might not have the same protection as the original server structure <br/> 4. The browse queue management is now split between different data structures, potentially causing inconsistencies |
| [_mdns_browse_send](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7440) | [browse_send](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L80) | The refactored function replaces _mdns_search_send_pcb with mdns_priv_query_send, which changes the PCB readiness check from mdns_is_netif_ready(tcpip_if, ip_protocol) && _mdns_server->interfaces[tcpip_if].pcbs[ip_protocol].state > PCB_INIT to mdsn_priv_pcb_is_inited(tcpip_if, ip_protocol). This could change when queries are sent and there's a potential typo in "mdsn_priv_pcb_is_inited" (should likely be "mdns_priv_pcb_is_inited"). Additionally, the packet management in mdns_priv_query_send (create, dispatch, immediate free) may have different error handling characteristics than the original implementation. |
| [_mdns_add_browse_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7461) | [add_browse_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L288) | The function visibility has changed from static to potentially non-static (the refactored code snippet doesn't show the static keyword). This could lead to linkage issues or unintended external access. Additionally, all call sites in the original codebase need to be verified to ensure they now call the renamed function in the new file location (mdns_browser.c vs mdns.c). |
| [_mdns_browse_result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7488) | [mdns_priv_browse_result_add_ip](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L315) | The refactored function replaces `_mdns_add_browse_result` with `add_browse_result` without the `_mdns_` prefix, which may indicate a missing namespace or different function implementation. Additionally, the TTL update logic changed from `_mdns_result_update_ttl` to `mdns_priv_query_update_result_ttl`, which could have different behavior for TTL calculations. The search results show no matches for these renamed functions in the refactored codebase, suggesting they may be missing or have different signatures/implementations. |
| [_mdns_browse_find_from](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7553) | [mdns_priv_browse_find](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L205) | The refactored function changed from taking a browse list parameter to using a global variable 's_browse', which could introduce thread safety issues and makes the function dependent on global state. Additionally, the original function was called with '_mdns_server->browse' as parameter, but the refactored version directly uses 's_browse' without clear evidence that these are equivalent. This change in data access pattern could lead to incorrect browse list traversal if the global variable is not properly synchronized with the original server state. |
| [is_txt_item_in_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7583) | [is_txt_item_in_list](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L376) | The refactored function appears to be identical to the original (100% similarity) but has been moved from mdns.c to mdns_browser.c. This change in location could introduce subtle issues if: <br/> 1. The function now has different visibility/scope in the new file <br/> 2. Callers in the original location (mdns.c) cannot access it if it remains static <br/> 3. The function assumes different context or dependencies in the browser component vs responder component <br/> 4. The TXT item comparison logic might need different behavior between browser and responder contexts |
| [_mdns_browse_result_add_txt](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7601) | ??? | |
| [_mdns_copy_address_in_previous_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7699) | [copy_address_in_previous_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L492) | |
| [_mdns_browse_result_add_srv](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7720) | [mdns_priv_browse_result_add_srv](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L513) | The refactored function calls two helper functions without the expected 'mdns_priv_' prefix: 'copy_address_in_previous_result' and 'add_browse_result'. This naming inconsistency suggests these functions may not be properly declared or implemented in the refactored codebase, potentially leading to linker errors or incorrect function resolution. Additionally, the final explicit 'return' statement was removed, which could affect code clarity but likely doesn't impact functionality. |
| [_mdns_browse_sync](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7800) | [browse_sync](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_browser.c#L59) | |
| [_debug_printf_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7821) | [dbg_printf_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_debug.c#L367) | The refactored function dbg_printf_result has been changed from non-static to static, which may break external calls if the function was used outside its compilation unit. Additionally, the addition of mdns_dbg_flush() at the end changes the debug output behavior - it now forces immediate flushing which could impact performance in debug-heavy scenarios or embedded systems with constrained I/O resources. |
| [debug_printf_browse_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7852) | [mdns_debug_printf_browse_result](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_debug.c#L399) | The refactored function adds an unconditional call to mdns_dbg_flush() which was not present in the original function. This could potentially impact performance in debug mode, especially if called frequently, as it may force immediate output flushing that wasn't happening before. Additionally, the debug macro names have changed from _mdns_dbg_printf/_debug_printf_result to dbg_printf/dbg_printf_result, which could have different formatting or behavior if these macros are not functionally equivalent. |
| [debug_printf_browse_result_all](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L7859) | [mdns_debug_printf_browse_result_all](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_debug.c#L407) | The refactored function adds a call to mdns_dbg_flush() at the end. While this might be intentional to ensure debug output is flushed, it could introduce timing differences in debugging scenarios or potentially block if the flush operation has significant overhead. In embedded systems or real-time environments, this could affect system behavior during debugging sessions. The original function did not have any flush operation, so output buffering behavior may have changed. |
Reference in New Issue Copy Permalink