net: mdns: add capability of providing records in runtime #60271

konradderda · 2023-07-11T20:39:18Z

Current implementation of mDNS responder does makes it mandatory to have all the records set at compile time. It is not suited well for applications that have to publish/unpublish or change records in runtime, e.g. data received from the network.

github-actions

Hello @konradderda, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

cfriedt · 2023-07-12T12:23:46Z

Current implementation of mDNS responder does makes it mandatory to have all the records set at compile time.

Technically, that's not exactly the case, it only requires the pointers to memory for DNS records to be set at compile time (and subsequently the number of records).

It is not suited well for applications that have to publish/unpublish or change records in runtime, e.g. data received from the network.

The memory that each record points to can be runtime modifiable today.

In any case though, dns_sd is definitely ripe for improvement.

cfriedt

Looks good to me, with a couple of minor modifications.

Please fix the compliance issues and ensure the build is green.

If you have interest in this area, I would recommend checking out some of the items on the TODO list here #38846

cfriedt · 2023-07-12T12:25:23Z

subsys/net/lib/dns/mdns_responder.c

-	DNS_SD_FOREACH(record) {
+	DNS_SD_COUNT(&rec_num);
+
+	while (rec_num || ext_rec_num) {


Please use integer comparisons here instead of treating the integers as bools

cfriedt · 2023-07-12T12:29:49Z

cc @pdgendt

rlubos

Please check the compliance results, it reported some nits as well. Otherwise looks good.

rlubos · 2023-07-12T14:25:07Z

include/zephyr/net/mdns_responder.h

+
+#include <stddef.h>
+
+#include "dns_sd.h"


#include <zephyr/net/dns_sd.h>

rlubos · 2023-07-12T14:25:37Z

include/zephyr/net/mdns_responder.h

+ * @ref DNS_SD_REGISTER_SERVICE (if any) and then go over external records.
+ *
+ * @param records An array of mDNS records
+ * @param count The number of elemnts


Suggested change

* @param count The number of elemnts

* @param count The number of elements

rlubos · 2023-07-12T14:26:31Z

include/zephyr/net/mdns_responder.h

+ *
+ * @param records An array of mDNS records
+ * @param count The number of elemnts
+ * @return int 0 for OK; -EINVAL for invalid parameters.


@return doesn't need type

konradderda · 2023-08-03T19:15:56Z

Hello @cfriedt,
Thank you for your comments

The memory that each record points to can be runtime modifiable today.

That was my first idea. However, it's not possible because the text_size field of dns_sd_rec is not a pointer so it would have a fixed value and cannot be modified:

zephyr/include/zephyr/net/dns_sd.h

Line 233 in e544bdb

size_t text_size;

Moreover, such approach would be enough for a small number of records. With this approach the user can provide a pointer to a static array, memory slab, heap allocated memory, etc.

rlubos

Looking at the doc build output, it looks that we needed to escape square brackets in struct dns_sd_rec documentation (i. e. < >)

konradderda · 2023-08-17T09:55:23Z

@cfriedt @ssharks @tbursztyka, sorry for bothering you but it seems that this PR could be closed with an additional approval, could you please look at it? Thanks in advance.

ssharks

The only thing I observe: Is it clear to the users that the mdns_responder_set_ext_records knows it have to make the data stay at that location instead of providing a stack variable?

konradderda · 2023-08-22T06:12:26Z

The only thing I observe: Is it clear to the users that the mdns_responder_set_ext_records knows it have to make the data stay at that location instead of providing a stack variable?

@ssharks, Thank you for the question, I think that providing a stack variable is perfectly fine as long as the scope is prolonged - e.g. we can have an array of dns_sd_rec in the main() function with while (true) loop. Another example - delegating k_work to publish records only once for some time (e.g. for 5 minutes after initialization) where there is no point to occupy the RAM for whole time. These are not common use cases and of course one can use other solutions but I believe there is no need of enforcing protection from this (+ it feels like a common sense, like not returning pointers to stack variables).

include/zephyr/net/mdns_responder.h

jukkar

Some changes needed

github-actions · 2023-12-03T00:30:24Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

jukkar · 2023-12-04T08:31:21Z

ping @konradderda, just wondering if you would have some time to address the latest comments?

konradderda · 2023-12-21T15:05:47Z

@jukkar I am sorry for the delay. There is no problem with addressing the comments regarding the code. However, I had tried to write a test case for my change and I found that with the current implementation it's quite hard to do. Unfortunately, at the moment I will not be able to provide a proper test implementation.

MaureenHelm · 2024-01-31T21:42:20Z

@konradderda will you return to this before the 3.6 merge window closes this week?

jukkar · 2024-03-05T08:00:48Z

ping @konradderda, are you able to continue with this? The needed changes look rather small and it would be nice to get this merged as this has been pending for a long time. It would be also nicer to get this in at the beginning of the 3.7 release cycle than at the end.

jukkar · 2024-03-05T08:03:19Z

The needed changes look rather small

Hmm, I missed the fact that unit tests would need changes too so the amount of work is bigger.

konradderda · 2024-03-07T11:26:27Z

@jukkar @MaureenHelm I had finally got some time and prepared a test for this change with a foundations for implementation of new test cases in the futute. Also, I have applied last remarks from @cfriedt .

jukkar · 2024-03-07T13:19:39Z

tests/net/lib/mdns_responder/src/main.c

+	if (!ifaddr) {
+		zassert_not_null(ifaddr, "Failed to add LL-addr");
+	} else {
+		/* we need to set the addresses preferred */
+		ifaddr->addr_state = NET_ADDR_PREFERRED;
+	}


We can simplify this to:

zassert_not_null(ifaddr, "Failed to add LL-addr"); /* we need to set the addresses preferred */ ifaddr->addr_state = NET_ADDR_PREFERRED;

because if the zassert fails, then the program stops so there is no need to have the if() else() branches.

jukkar · 2024-03-07T13:24:41Z

subsys/net/lib/dns/mdns_responder.c

+		 */
+		if (rec_num > 0) {
+			DNS_SD_GET(rec_num - 1, &record);
+			rec_num -= 1;


That is perfectly valid code but usually -1 is done via rec_num--;

jukkar · 2024-03-07T13:24:56Z

subsys/net/lib/dns/mdns_responder.c

+			rec_num -= 1;
+		} else {
+			record = &external_records[ext_rec_num - 1];
+			ext_rec_num -= 1;


and ext_rec_num--;

subsys/net/lib/dns/mdns_responder.c

Current implementation of mDNS responder does makes it mandatory to have all the records set at compile time. It is not suited well for applications that have to publish/unpublish or change records in runtime, e.g. data received from the network. Signed-off-by: Konrad Derda <konrad.derda@nordicsemi.no>

This commit adds a test for externally stored mDNS records for mDNS responder. Signed-off-by: Konrad Derda <konrad.derda@nordicsemi.no>

Outdated

konradderda requested review from rlubos, tbursztyka and cfriedt as code owners July 11, 2023 20:39

zephyrbot added the area: Networking label Jul 11, 2023

zephyrbot requested a review from ssharks July 11, 2023 20:39

zephyrbot assigned rlubos Jul 11, 2023

github-actions bot reviewed Jul 11, 2023

View reviewed changes

cfriedt reviewed Jul 12, 2023

View reviewed changes

rlubos reviewed Jul 12, 2023

View reviewed changes

konradderda force-pushed the mdns_external_records branch from 731b176 to 9132a20 Compare August 3, 2023 19:05

konradderda force-pushed the mdns_external_records branch 2 times, most recently from 261f0fc to b8c69a8 Compare August 4, 2023 06:41

rlubos reviewed Aug 4, 2023

View reviewed changes

konradderda force-pushed the mdns_external_records branch from b8c69a8 to ebef21e Compare August 7, 2023 06:24

konradderda requested review from rlubos and cfriedt August 7, 2023 08:38

rlubos previously approved these changes Aug 7, 2023

View reviewed changes

ssharks reviewed Aug 20, 2023

View reviewed changes

ssharks previously approved these changes Aug 22, 2023

View reviewed changes

jukkar reviewed Sep 7, 2023

View reviewed changes

include/zephyr/net/mdns_responder.h Show resolved Hide resolved

include/zephyr/net/mdns_responder.h Outdated Show resolved Hide resolved

include/zephyr/net/mdns_responder.h Outdated Show resolved Hide resolved

jukkar requested changes Sep 7, 2023

View reviewed changes

konradderda dismissed stale reviews from ssharks and rlubos via 7c707ec September 11, 2023 10:25

konradderda force-pushed the mdns_external_records branch from ebef21e to 7c707ec Compare September 11, 2023 10:25

konradderda requested a review from jukkar September 11, 2023 10:26

fabiobaltieri added this to the v3.6.0 milestone Oct 3, 2023

github-actions bot added the Stale label Dec 3, 2023

jukkar removed the Stale label Dec 3, 2023

henrikbrixandersen removed this from the v3.6.0 milestone Feb 7, 2024

konradderda dismissed jukkar’s stale review via ca4fa5c March 7, 2024 11:20

konradderda force-pushed the mdns_external_records branch from 705f986 to ca4fa5c Compare March 7, 2024 11:20

zephyrbot requested a review from pdgendt March 7, 2024 11:20

konradderda force-pushed the mdns_external_records branch from ca4fa5c to 795cbfd Compare March 7, 2024 11:24

konradderda force-pushed the mdns_external_records branch from 795cbfd to 1d4de57 Compare March 7, 2024 11:28

konradderda requested review from cfriedt and jukkar March 7, 2024 11:29

konradderda force-pushed the mdns_external_records branch 3 times, most recently from 342c82a to 9d681cb Compare March 7, 2024 11:38

jukkar reviewed Mar 7, 2024

View reviewed changes

konradderda force-pushed the mdns_external_records branch from 9d681cb to 4b19cdb Compare March 21, 2024 18:54

konradderda added 2 commits March 21, 2024 19:55

tests: net: mdns: add test for external mDNS records

9ed5102

This commit adds a test for externally stored mDNS records for mDNS responder. Signed-off-by: Konrad Derda <konrad.derda@nordicsemi.no>

konradderda force-pushed the mdns_external_records branch from 4b19cdb to 9ed5102 Compare March 21, 2024 18:56

rlubos approved these changes Mar 22, 2024

View reviewed changes

jukkar approved these changes Mar 22, 2024

View reviewed changes

fabiobaltieri merged commit 5e46a49 into zephyrproject-rtos:main Mar 22, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: mdns: add capability of providing records in runtime #60271

net: mdns: add capability of providing records in runtime #60271

konradderda commented Jul 11, 2023

github-actions bot left a comment

cfriedt commented Jul 12, 2023

cfriedt left a comment •

edited

cfriedt Jul 12, 2023

cfriedt commented Jul 12, 2023

rlubos left a comment

rlubos Jul 12, 2023

rlubos Jul 12, 2023

rlubos Jul 12, 2023

konradderda commented Aug 3, 2023

rlubos left a comment

konradderda commented Aug 17, 2023

ssharks left a comment

konradderda commented Aug 22, 2023

jukkar left a comment

github-actions bot commented Dec 3, 2023

jukkar commented Dec 4, 2023

konradderda commented Dec 21, 2023

MaureenHelm commented Jan 31, 2024

jukkar commented Mar 5, 2024

jukkar commented Mar 5, 2024

konradderda commented Mar 7, 2024

jukkar Mar 7, 2024

konradderda Mar 21, 2024

jukkar Mar 7, 2024

konradderda Mar 21, 2024

jukkar Mar 7, 2024

konradderda Mar 21, 2024

	* @param count The number of elemnts
	* @param count The number of elements


		#include <stddef.h>

		#include "dns_sd.h"

net: mdns: add capability of providing records in runtime #60271

net: mdns: add capability of providing records in runtime #60271

Conversation

konradderda commented Jul 11, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

cfriedt commented Jul 12, 2023

cfriedt left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt commented Jul 12, 2023

rlubos 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

konradderda commented Aug 3, 2023

rlubos left a comment

Choose a reason for hiding this comment

konradderda commented Aug 17, 2023

ssharks left a comment

Choose a reason for hiding this comment

konradderda commented Aug 22, 2023

jukkar left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2023

jukkar commented Dec 4, 2023

konradderda commented Dec 21, 2023

MaureenHelm commented Jan 31, 2024

jukkar commented Mar 5, 2024

jukkar commented Mar 5, 2024

konradderda commented Mar 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt left a comment •

edited