fix(mdns): fix compiling issue when disabling IPv4 (IDFGH-11453) #414

wqx6 · 2023-11-03T10:00:13Z

IDF v5.1 provides a menuconfig option to disable IPv4 in LwIP. And mdns component compiling would fail when disabling IPv4

igrr · 2023-11-14T12:28:45Z

It would be nice to also add an sdkconfig.ci.* configuration with IPV4 disabled, so that at least the builds for this use case are validated in CI.

david-cermak · 2023-11-14T12:55:50Z

components/mdns/mdns.c

@@ -17,6 +17,7 @@
 #include "mdns_networking.h"
 #include "esp_log.h"
 #include "esp_random.h"
+#include "lwip/opt.h"


Please don't use lwip headers from this file, it's supposed to be network stack independent.

You can use CONIG_LWIP_IPV4 instead or define a MNDS_IPV4 based on the config option in mdns_config.h

CONFIG_LWIP_IPV4 was introduced in IDF v5.1 but this managed component is for IDF version>=5.0, so I didn't use this config option here. May I create a menuconfig option in this component instead of using CONIG_LWIP_IPV4?

wqx6 · 2023-11-16T10:56:27Z

It would be nice to also add an sdkconfig.ci.* configuration with IPV4 disabled, so that at least the builds for this use case are validated in CI.

Added an configuration file that disables IPv4.

david-cermak

LGTM in general.

The only concern is the LWIP dependency that you're bringing to everything or introduction of our own config options.

I cannot think of a decently elegant way to address this issue, but probably creating definitions of MDNS_IPV4/6 in mdns_config.h based on global config options and IDF release versions.

components/mdns/Kconfig

david-cermak · 2023-11-16T11:32:36Z

components/mdns/mdns.c

@@ -185,7 +185,7 @@ static inline void _mdns_clean_netif_ptr(mdns_if_t tcpip_if)
 static mdns_if_t _mdns_get_if_from_esp_netif(esp_netif_t *esp_netif)
 {
    for (int i = 0; i < MDNS_MAX_INTERFACES; ++i) {
-        if (esp_netif == s_esp_netifs[i].netif) {
+        if (esp_netif == s_esp_netifs[i].netif || (s_esp_netifs[i].predefined && esp_netif == esp_netif_from_preset_if(s_esp_netifs[i].predef_if))) {


Could you please explain the reason behind this change? I thought the static array already contains the predef netif, correct?

The predefined netifs in the static array are NULL when firstly calling this function if IPv4 is disabled, and the mld6_joingroup_netif will not be called rightly then.

Added comment here/

The predefined netifs in the static array are NULL

@gabsuren Could you please check?

@wqx6 Do you use the predefined interfaces (default STA, AP or Ethernet) or do you initialize netifs manually?

Yes, I used predefined STA interface

components/mdns/mdns.c

components/mdns/mdns_networking_lwip.c

components/mdns/mdns_networking_socket.c

wqx6 · 2023-11-20T11:30:11Z

The CI fails for socket example, but I can resolve the hostnames with the socket sdkconfig locally. @david-cermak

wqx6 · 2023-12-12T04:24:28Z

Could you please try rerunning it a few times to see if you can reproduce the issue?

I have run the pytest script for over 10 times, but none of them failed.

chshu · 2024-01-25T08:55:16Z

@david-cermak @gabsuren The previous CI issue has been fixed. Could you review and merge it if no other comments? We also want to make a new mDNS release to publish this change, it's required by the Matter Sleep example for C6.

gabsuren · 2024-01-25T09:28:24Z

components/mdns/examples/query_advertise/pytest_mdns.py

@@ -64,6 +64,7 @@ def mdns_server(esp_host, events):
    sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
    sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
    sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1)
+    sock.setsockopt(socket.SOL_SOCKET, socket.SO_BINDTODEVICE, 'eth0\0'.encode('utf-8'))


Why we are binding it to eth0 only?

I think the pytest script is using eth0 to communicate with the test board on CI runner.

If I didn't bind the interface, CI will fail https://github.com/espressif/esp-protocols/actions/runs/7550392768/job/20556119271

There are two things concerning with this change.

The tests may fail if we switch to a different interface or operating system, such as macOS or Windows. I can address this issue later by updating pytest myself.

More worryingly, it's unclear why the tests were passing previously without being bound to the specific interface. This raises questions about potential breaking changes that could arise from this discrepancy.

@wqx6 I have executed the changes without binding, in separate PR503 and it passed
https://github.com/espressif/esp-protocols/actions/runs/7783718151

So I suppose this change is not necessary . Could you please, revert this and rebase with master so we can merge it?

david-cermak · 2024-01-26T11:31:48Z

components/mdns/mdns_networking_socket.c

-        mdns_ip_protocol_t other_ip_proto = ip_protocol == MDNS_IP_PROTOCOL_V4 ? MDNS_IP_PROTOCOL_V6 : MDNS_IP_PROTOCOL_V4;
-        int err = join_mdns_multicast_group(sock, netif, other_ip_proto);
-        if (err < 0) {
-            ESP_LOGE(TAG, "Failed to add ipv6 multicast group for protocol %d", ip_protocol);
-            return false;
-        }
-        s_interfaces[tcpip_if].proto |= (other_ip_proto == MDNS_IP_PROTOCOL_V4 ? PROTO_IPV4 : PROTO_IPV6);


I wonder why this was removed? The idea is that we already have a socket supporting one IP protocol (say IPv4) and someone calls create_pcb(same_netif, IPv6). Then we'll never join the IPv6 multicast group, correct?

The create_pcb(some_netif, IPv6) will still call join_mdns_multicast_group(sock, netif, IPv6); in the following code.

But if we keep the original logic, When we already have a socket supporting IPv4, and then someone calls create_pcb(some_netif, IPv4), the netif will never join IPv4 multicast group.

Oh, I see. so you actually always join the multicast group (again).
This would (probably?) work as we guard this in mdns.c using mdns_is_netif_ready() API.

david-cermak

changes LGTM, provided that the socket networking part has been tested with IPv4 only and dual stack.

wqx6 force-pushed the fix/mdns_disable_ipv4 branch 2 times, most recently from fd080cd to b02c99d Compare November 3, 2023 11:25

wqx6 force-pushed the fix/mdns_disable_ipv4 branch from b02c99d to b5a29ae Compare November 13, 2023 03:01

gabsuren added the mdns label Nov 14, 2023

david-cermak self-requested a review November 14, 2023 12:23

david-cermak assigned gabsuren Nov 14, 2023

david-cermak requested review from gabsuren, espressif-abhikroy and euripedesrocha November 14, 2023 12:23

espressif-bot added the Status: Opened label Nov 14, 2023

github-actions bot changed the title ~~fix(mdns): fix compiling issue when disabling IPv4~~ fix(mdns): fix compiling issue when disabling IPv4 (IDFGH-11453) Nov 14, 2023

david-cermak reviewed Nov 14, 2023

View reviewed changes

wqx6 force-pushed the fix/mdns_disable_ipv4 branch 2 times, most recently from 9a490aa to 562987d Compare November 16, 2023 10:46

david-cermak requested a review from kostaond November 16, 2023 11:16

david-cermak reviewed Nov 16, 2023

View reviewed changes

wqx6 force-pushed the fix/mdns_disable_ipv4 branch 10 times, most recently from 86e600d to 92883ce Compare November 20, 2023 03:00

espressif-bot unassigned gabsuren Nov 20, 2023

wqx6 force-pushed the fix/mdns_disable_ipv4 branch from ba002a9 to d4eb6a7 Compare December 12, 2023 04:24

wqx6 force-pushed the fix/mdns_disable_ipv4 branch 10 times, most recently from 8e26ccb to b67aeec Compare January 10, 2024 10:43

wqx6 force-pushed the fix/mdns_disable_ipv4 branch 2 times, most recently from 6cbb5c4 to 41a9c9c Compare January 15, 2024 11:48

wqx6 added 2 commits January 17, 2024 10:48

fix(mdns): fix compiling issue when disabling IPv4

2646dcd

fix(mdns): fix the logic of creating pcb for networking socket

5000a9a

wqx6 force-pushed the fix/mdns_disable_ipv4 branch from 41a9c9c to 5000a9a Compare January 17, 2024 02:48

gabsuren reviewed Jan 25, 2024

View reviewed changes

david-cermak reviewed Jan 26, 2024

View reviewed changes

david-cermak approved these changes Jan 30, 2024

View reviewed changes

espressif-bot added Status: Reviewing and removed Status: In Progress labels Feb 5, 2024

wqx6 force-pushed the fix/mdns_disable_ipv4 branch from 0e29823 to 5000a9a Compare February 6, 2024 07:09

gabsuren self-requested a review February 6, 2024 07:44

gabsuren approved these changes Feb 6, 2024

View reviewed changes

gabsuren merged commit 891384c into espressif:master Feb 6, 2024
59 of 60 checks passed

espressif-bot added Status: Done and removed Status: Reviewing labels Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mdns): fix compiling issue when disabling IPv4 (IDFGH-11453) #414

fix(mdns): fix compiling issue when disabling IPv4 (IDFGH-11453) #414

wqx6 commented Nov 3, 2023 •

edited

Loading

igrr commented Nov 14, 2023

david-cermak Nov 14, 2023

wqx6 Nov 16, 2023

wqx6 commented Nov 16, 2023

david-cermak left a comment

david-cermak Nov 16, 2023

wqx6 Nov 17, 2023

wqx6 Nov 17, 2023

david-cermak Nov 22, 2023

wqx6 Nov 24, 2023

wqx6 commented Nov 20, 2023

wqx6 commented Dec 12, 2023

chshu commented Jan 25, 2024

gabsuren Jan 25, 2024

wqx6 Jan 26, 2024

wqx6 Jan 26, 2024

gabsuren Feb 5, 2024

gabsuren Feb 5, 2024 •

edited

Loading

wqx6 Feb 6, 2024

gabsuren Feb 6, 2024

david-cermak Jan 26, 2024

wqx6 Jan 30, 2024 •

edited

Loading

david-cermak Jan 30, 2024

david-cermak left a comment

fix(mdns): fix compiling issue when disabling IPv4 (IDFGH-11453) #414

fix(mdns): fix compiling issue when disabling IPv4 (IDFGH-11453) #414

Conversation

wqx6 commented Nov 3, 2023 • edited Loading

igrr commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wqx6 commented Nov 16, 2023

david-cermak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wqx6 commented Nov 20, 2023

wqx6 commented Dec 12, 2023

chshu commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabsuren Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wqx6 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-cermak left a comment

Choose a reason for hiding this comment

wqx6 commented Nov 3, 2023 •

edited

Loading

gabsuren Feb 5, 2024 •

edited

Loading

wqx6 Jan 30, 2024 •

edited

Loading