[PATCH] src: configure systemd-resolved's MulticastDNS= setting

[PATCH] src: configure systemd-resolved's MulticastDNS= setting

[Daniel Lin]

[-- Attachment #1: Type: text/plain, Size: 7227 bytes --]

When using iwd.conf:[General].EnableNetworkConfiguration=true, it is not
possible to configure systemd.network:[Network].MulticastDNS= as
systemd-networkd considers the link to be unmanaged. This patch allows
iwd to configure that setting on systemd-resolved directly.
---
 src/iwd.config.rst |  7 ++++
 src/netconfig.c    |  6 ++++
 src/resolve.c      | 83 ++++++++++++++++++++++++++++++++--------------
 src/resolve.h      |  1 +
 4 files changed, 73 insertions(+), 24 deletions(-)

diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 0064dfd2..73d41cc0 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -212,6 +212,13 @@ The group ``[Network]`` contains network configuration related settings.
 
        If not specified, ``300`` is used as default.
 
+   * - MulticastDNS
+     - Values: true, false, resolve
+
+       Configures multicast DNS. See ``man 5 systemd.network`` for details.
+
+       Only applies when ``NameResolvingService=systemd``.
+
 Blacklist
 ---------
 
diff --git a/src/netconfig.c b/src/netconfig.c
index cf092b78..749ba0ad 100644
--- a/src/netconfig.c
+++ b/src/netconfig.c
@@ -963,6 +963,8 @@ bool netconfig_configure(struct netconfig *netconfig,
 				const uint8_t *mac_address,
 				netconfig_notify_func_t notify, void *user_data)
 {
+	char *mdns;
+
 	netconfig->dns4_overrides = l_settings_get_string_list(active_settings,
 							"IPv4", "DNS", ' ');
 
@@ -1006,6 +1008,10 @@ bool netconfig_configure(struct netconfig *netconfig,
 
 	netconfig_ipv6_select_and_install(netconfig);
 
+	mdns = l_settings_get_string(active_settings, "Network", "MulticastDNS");
+	resolve_set_mdns(netconfig->resolve, mdns);
+	l_free(mdns);
+
 	return true;
 }
 
diff --git a/src/resolve.c b/src/resolve.c
index d3b483a8..993bbf34 100644
--- a/src/resolve.c
+++ b/src/resolve.c
@@ -40,6 +40,7 @@
 struct resolve_ops {
 	void (*set_dns)(struct resolve *resolve, char **dns_list);
 	void (*set_domains)(struct resolve *resolve, char **domain_list);
+	void (*set_mdns)(struct resolve *resolve, char *mdns);
 	void (*revert)(struct resolve *resolve);
 	void (*destroy)(struct resolve *resolve);
 };
@@ -78,6 +79,17 @@ void resolve_set_domains(struct resolve *resolve, char **domain_list)
 	resolve->ops->set_domains(resolve, domain_list);
 }
 
+void resolve_set_mdns(struct resolve *resolve, char *mdns)
+{
+	if (!mdns)
+		return;
+
+	if (!resolve->ops->set_mdns)
+		return;
+
+	resolve->ops->set_mdns(resolve, mdns);
+}
+
 void resolve_revert(struct resolve *resolve)
 {
 	if (!resolve->ops->revert)
@@ -112,9 +124,10 @@ struct systemd {
 	struct resolve super;
 };
 
-static void systemd_link_dns_reply(struct l_dbus_message *message,
+static void systemd_link_generic_reply(struct l_dbus_message *message,
 								void *user_data)
 {
+	const char *type = user_data;
 	const char *name;
 	const char *text;
 
@@ -123,8 +136,8 @@ static void systemd_link_dns_reply(struct l_dbus_message *message,
 
 	l_dbus_message_get_error(message, &name, &text);
 
-	l_error("resolve-systemd: Failed to modify the DNS entries. %s: %s",
-								name, text);
+	l_error("resolve-systemd: Failed to modify the %s entries. %s: %s",
+								type, name, text);
 }
 
 static bool systemd_builder_add_dns(struct l_dbus_message_builder *builder,
@@ -205,23 +218,8 @@ static void resolve_systemd_set_dns(struct resolve *resolve, char **dns_list)
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
-	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
-								NULL, NULL);
-}
-
-static void systemd_set_link_domains_reply(struct l_dbus_message *message,
-								void *user_data)
-{
-	const char *name;
-	const char *text;
-
-	if (!l_dbus_message_is_error(message))
-		return;
-
-	l_dbus_message_get_error(message, &name, &text);
-
-	l_error("resolve-systemd: Failed to modify the domain entries. %s: %s",
-								name, text);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"DNS", NULL);
 }
 
 static void resolve_systemd_set_domains(struct resolve *resolve,
@@ -266,8 +264,43 @@ static void resolve_systemd_set_domains(struct resolve *resolve,
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
-	l_dbus_send_with_reply(dbus_get_bus(), message,
-				systemd_set_link_domains_reply, NULL, NULL);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+				"domains", NULL);
+}
+
+static void resolve_systemd_set_mdns(struct resolve *resolve, char *mdns)
+{
+	struct l_dbus_message_builder *builder;
+	struct l_dbus_message *message;
+
+	l_debug("ifindex: %u", resolve->ifindex);
+
+	if (L_WARN_ON(!systemd_state.is_ready))
+		return;
+
+	message = l_dbus_message_new_method_call(dbus_get_bus(),
+					SYSTEMD_RESOLVED_SERVICE,
+					SYSTEMD_RESOLVED_MANAGER_PATH,
+					SYSTEMD_RESOLVED_MANAGER_INTERFACE,
+					"SetLinkMulticastDNS");
+
+	if (!message)
+		return;
+
+	builder = l_dbus_message_builder_new(message);
+	if (!builder) {
+		l_dbus_message_unref(message);
+		return;
+	}
+
+	l_dbus_message_builder_append_basic(builder, 'i', &resolve->ifindex);
+	l_dbus_message_builder_append_basic(builder, 's', mdns);
+
+	l_dbus_message_builder_finalize(builder);
+	l_dbus_message_builder_destroy(builder);
+
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"MulticastDNS", NULL);
 }
 
 static void resolve_systemd_revert(struct resolve *resolve)
@@ -288,8 +321,8 @@ static void resolve_systemd_revert(struct resolve *resolve)
 		return;
 
 	l_dbus_message_set_arguments(message, "i", resolve->ifindex);
-	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
-								NULL, NULL);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"DNS", NULL);
 }
 
 static void resolve_systemd_destroy(struct resolve *resolve)
@@ -302,6 +335,7 @@ static void resolve_systemd_destroy(struct resolve *resolve)
 static const struct resolve_ops systemd_ops = {
 	.set_dns = resolve_systemd_set_dns,
 	.set_domains = resolve_systemd_set_domains,
+	.set_mdns = resolve_systemd_set_mdns,
 	.revert = resolve_systemd_revert,
 	.destroy = resolve_systemd_destroy,
 };
@@ -482,6 +516,7 @@ static void resolve_resolvconf_destroy(struct resolve *resolve)
 static struct resolve_ops resolvconf_ops = {
 	.set_dns = resolve_resolvconf_set_dns,
 	.set_domains = resolve_resolvconf_set_domains,
+	.set_mdns = NULL,
 	.revert = resolve_resolvconf_revert,
 	.destroy = resolve_resolvconf_destroy,
 };
diff --git a/src/resolve.h b/src/resolve.h
index 64ed15f3..09cd9ac2 100644
--- a/src/resolve.h
+++ b/src/resolve.h
@@ -23,5 +23,6 @@
 struct resolve *resolve_new(uint32_t ifindex);
 void resolve_set_dns(struct resolve *resolve, char **dns_list);
 void resolve_set_domains(struct resolve *resolve, char **domain_list);
+void resolve_set_mdns(struct resolve *resolve, char *mdns);
 void resolve_revert(struct resolve *resolve);
 void resolve_free(struct resolve *resolve);
-- 
2.30.1

Re: [PATCH] src: configure systemd-resolved's MulticastDNS= setting

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 8293 bytes --]

Hi Daniel,

On 2/10/21 5:43 PM, Daniel Lin wrote:
> When using iwd.conf:[General].EnableNetworkConfiguration=true, it is not
> possible to configure systemd.network:[Network].MulticastDNS= as
> systemd-networkd considers the link to be unmanaged. This patch allows
> iwd to configure that setting on systemd-resolved directly.
> ---
>   src/iwd.config.rst |  7 ++++
>   src/netconfig.c    |  6 ++++
>   src/resolve.c      | 83 ++++++++++++++++++++++++++++++++--------------
>   src/resolve.h      |  1 +
>   4 files changed, 73 insertions(+), 24 deletions(-)
> 
> diff --git a/src/iwd.config.rst b/src/iwd.config.rst
> index 0064dfd2..73d41cc0 100644
> --- a/src/iwd.config.rst
> +++ b/src/iwd.config.rst
> @@ -212,6 +212,13 @@ The group ``[Network]`` contains network configuration related settings.
>   
>          If not specified, ``300`` is used as default.
>   
> +   * - MulticastDNS
> +     - Values: true, false, resolve

Is there a default?  If so, it should be in bold.  What happens if this setting 
is omitted?

> +
> +       Configures multicast DNS. See ``man 5 systemd.network`` for details.
> +
> +       Only applies when ``NameResolvingService=systemd``.
> +
>   Blacklist
>   ---------
>   
> diff --git a/src/netconfig.c b/src/netconfig.c
> index cf092b78..749ba0ad 100644
> --- a/src/netconfig.c
> +++ b/src/netconfig.c
> @@ -963,6 +963,8 @@ bool netconfig_configure(struct netconfig *netconfig,
>   				const uint8_t *mac_address,
>   				netconfig_notify_func_t notify, void *user_data)
>   {
> +	char *mdns;
> +
>   	netconfig->dns4_overrides = l_settings_get_string_list(active_settings,
>   							"IPv4", "DNS", ' ');
>   
> @@ -1006,6 +1008,10 @@ bool netconfig_configure(struct netconfig *netconfig,
>   
>   	netconfig_ipv6_select_and_install(netconfig);
>   
> +	mdns = l_settings_get_string(active_settings, "Network", "MulticastDNS");

This can return NULL if no MulticastDNS setting is found.  What happens then?

> +	resolve_set_mdns(netconfig->resolve, mdns);
> +	l_free(mdns);
> +
>   	return true;
>   }
>   
> diff --git a/src/resolve.c b/src/resolve.c
> index d3b483a8..993bbf34 100644
> --- a/src/resolve.c
> +++ b/src/resolve.c
> @@ -40,6 +40,7 @@
>   struct resolve_ops {
>   	void (*set_dns)(struct resolve *resolve, char **dns_list);
>   	void (*set_domains)(struct resolve *resolve, char **domain_list);
> +	void (*set_mdns)(struct resolve *resolve, char *mdns);

should probably be const char *mdns

>   	void (*revert)(struct resolve *resolve);
>   	void (*destroy)(struct resolve *resolve);
>   };
> @@ -78,6 +79,17 @@ void resolve_set_domains(struct resolve *resolve, char **domain_list)
>   	resolve->ops->set_domains(resolve, domain_list);
>   }
>   
> +void resolve_set_mdns(struct resolve *resolve, char *mdns)
> +{
> +	if (!mdns)
> +		return;
> +
> +	if (!resolve->ops->set_mdns)
> +		return;
> +
> +	resolve->ops->set_mdns(resolve, mdns);
> +}
> +
>   void resolve_revert(struct resolve *resolve)
>   {
>   	if (!resolve->ops->revert)
> @@ -112,9 +124,10 @@ struct systemd {
>   	struct resolve super;
>   };
>   
> -static void systemd_link_dns_reply(struct l_dbus_message *message,
> +static void systemd_link_generic_reply(struct l_dbus_message *message,
>   								void *user_data)
>   {
> +	const char *type = user_data;
>   	const char *name;
>   	const char *text;
>   
> @@ -123,8 +136,8 @@ static void systemd_link_dns_reply(struct l_dbus_message *message,
>   
>   	l_dbus_message_get_error(message, &name, &text);
>   
> -	l_error("resolve-systemd: Failed to modify the DNS entries. %s: %s",
> -								name, text);
> +	l_error("resolve-systemd: Failed to modify the %s entries. %s: %s",
> +								type, name, text);
>   }
>   
>   static bool systemd_builder_add_dns(struct l_dbus_message_builder *builder,
> @@ -205,23 +218,8 @@ static void resolve_systemd_set_dns(struct resolve *resolve, char **dns_list)
>   	l_dbus_message_builder_finalize(builder);
>   	l_dbus_message_builder_destroy(builder);
>   
> -	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
> -								NULL, NULL);
> -}
> -
> -static void systemd_set_link_domains_reply(struct l_dbus_message *message,
> -								void *user_data)
> -{
> -	const char *name;
> -	const char *text;
> -
> -	if (!l_dbus_message_is_error(message))
> -		return;
> -
> -	l_dbus_message_get_error(message, &name, &text);
> -
> -	l_error("resolve-systemd: Failed to modify the domain entries. %s: %s",
> -								name, text);
> +	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
> +								"DNS", NULL);
>   }
>   
>   static void resolve_systemd_set_domains(struct resolve *resolve,
> @@ -266,8 +264,43 @@ static void resolve_systemd_set_domains(struct resolve *resolve,
>   	l_dbus_message_builder_finalize(builder);
>   	l_dbus_message_builder_destroy(builder);
>   
> -	l_dbus_send_with_reply(dbus_get_bus(), message,
> -				systemd_set_link_domains_reply, NULL, NULL);
> +	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
> +				"domains", NULL);
> +}
> +
> +static void resolve_systemd_set_mdns(struct resolve *resolve, char *mdns)
> +{
> +	struct l_dbus_message_builder *builder;
> +	struct l_dbus_message *message;
> +
> +	l_debug("ifindex: %u", resolve->ifindex);
> +
> +	if (L_WARN_ON(!systemd_state.is_ready))
> +		return;
> +
> +	message = l_dbus_message_new_method_call(dbus_get_bus(),
> +					SYSTEMD_RESOLVED_SERVICE,
> +					SYSTEMD_RESOLVED_MANAGER_PATH,
> +					SYSTEMD_RESOLVED_MANAGER_INTERFACE,
> +					"SetLinkMulticastDNS");
> +
> +	if (!message)
> +		return;
> +
> +	builder = l_dbus_message_builder_new(message);
> +	if (!builder) {
> +		l_dbus_message_unref(message);
> +		return;
> +	}
> +
> +	l_dbus_message_builder_append_basic(builder, 'i', &resolve->ifindex);
> +	l_dbus_message_builder_append_basic(builder, 's', mdns);
> +
> +	l_dbus_message_builder_finalize(builder);
> +	l_dbus_message_builder_destroy(builder);

Using a builder for something this simple is probably overkill.  You can replace 
the last ~12 lines by something like:

l_dbus_message_set_arguments(message, "is", resolve->ifindex, mdns);

> +
> +	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
> +								"MulticastDNS", NULL);
>   }
>   
>   static void resolve_systemd_revert(struct resolve *resolve)
> @@ -288,8 +321,8 @@ static void resolve_systemd_revert(struct resolve *resolve)
>   		return;
>   
>   	l_dbus_message_set_arguments(message, "i", resolve->ifindex);
> -	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
> -								NULL, NULL);
> +	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
> +								"DNS", NULL);
>   }
>   
>   static void resolve_systemd_destroy(struct resolve *resolve)
> @@ -302,6 +335,7 @@ static void resolve_systemd_destroy(struct resolve *resolve)
>   static const struct resolve_ops systemd_ops = {
>   	.set_dns = resolve_systemd_set_dns,
>   	.set_domains = resolve_systemd_set_domains,
> +	.set_mdns = resolve_systemd_set_mdns,
>   	.revert = resolve_systemd_revert,
>   	.destroy = resolve_systemd_destroy,
>   };
> @@ -482,6 +516,7 @@ static void resolve_resolvconf_destroy(struct resolve *resolve)
>   static struct resolve_ops resolvconf_ops = {
>   	.set_dns = resolve_resolvconf_set_dns,
>   	.set_domains = resolve_resolvconf_set_domains,
> +	.set_mdns = NULL,

This should already be NULL, so not needed and can be dropped

>   	.revert = resolve_resolvconf_revert,
>   	.destroy = resolve_resolvconf_destroy,
>   };
> diff --git a/src/resolve.h b/src/resolve.h
> index 64ed15f3..09cd9ac2 100644
> --- a/src/resolve.h
> +++ b/src/resolve.h
> @@ -23,5 +23,6 @@
>   struct resolve *resolve_new(uint32_t ifindex);
>   void resolve_set_dns(struct resolve *resolve, char **dns_list);
>   void resolve_set_domains(struct resolve *resolve, char **domain_list);
> +void resolve_set_mdns(struct resolve *resolve, char *mdns);
>   void resolve_revert(struct resolve *resolve);
>   void resolve_free(struct resolve *resolve);
> 

Regards,
-Denis

Re: [PATCH] src: configure systemd-resolved's MulticastDNS= setting

[Daniel Lin]

[-- Attachment #1: Type: text/plain, Size: 9537 bytes --]

On Thu, 2021-02-11 at 09:59 -0600, Denis Kenzior wrote:
> Hi Daniel,
> 
> On 2/10/21 5:43 PM, Daniel Lin wrote:
> > When using iwd.conf:[General].EnableNetworkConfiguration=true, it
> > is not
> > possible to configure systemd.network:[Network].MulticastDNS= as
> > systemd-networkd considers the link to be unmanaged. This patch
> > allows
> > iwd to configure that setting on systemd-resolved directly.
> > ---
> >   src/iwd.config.rst |  7 ++++
> >   src/netconfig.c    |  6 ++++
> >   src/resolve.c      | 83 ++++++++++++++++++++++++++++++++---------
> > -----
> >   src/resolve.h      |  1 +
> >   4 files changed, 73 insertions(+), 24 deletions(-)
> > 
> > diff --git a/src/iwd.config.rst b/src/iwd.config.rst
> > index 0064dfd2..73d41cc0 100644
> > --- a/src/iwd.config.rst
> > +++ b/src/iwd.config.rst
> > @@ -212,6 +212,13 @@ The group ``[Network]`` contains network
> > configuration related settings.
> >   
> >          If not specified, ``300`` is used as default.
> >   
> > +   * - MulticastDNS
> > +     - Values: true, false, resolve
> 
> Is there a default?  If so, it should be in bold.  What happens if
> this setting 
> is omitted?

If the setting is omitted, we don't touch the default value. Which
appears to be false, but somebody could patch systemd-resolved to
change that, so I wasn't sure if that should be documented here. I'll
add that though.

> 
> > +
> > +       Configures multicast DNS. See ``man 5 systemd.network`` for
> > details.
> > +
> > +       Only applies when ``NameResolvingService=systemd``.
> > +
> >   Blacklist
> >   ---------
> >   
> > diff --git a/src/netconfig.c b/src/netconfig.c
> > index cf092b78..749ba0ad 100644
> > --- a/src/netconfig.c
> > +++ b/src/netconfig.c
> > @@ -963,6 +963,8 @@ bool netconfig_configure(struct netconfig
> > *netconfig,
> >   				const uint8_t *mac_address,
> >   				netconfig_notify_func_t notify, void
> > *user_data)
> >   {
> > +	char *mdns;
> > +
> >   	netconfig->dns4_overrides =
> > l_settings_get_string_list(active_settings,
> >   							"IPv4", "DNS",
> > ' ');
> >   
> > @@ -1006,6 +1008,10 @@ bool netconfig_configure(struct netconfig
> > *netconfig,
> >   
> >   	netconfig_ipv6_select_and_install(netconfig);
> >   
> > +	mdns = l_settings_get_string(active_settings, "Network",
> > "MulticastDNS");
> 
> This can return NULL if no MulticastDNS setting is found.  What
> happens then?

Nothing happens, there's a null check in resolve_set_mdns().

> 
> > +	resolve_set_mdns(netconfig->resolve, mdns);
> > +	l_free(mdns);
> > +
> >   	return true;
> >   }
> >   
> > diff --git a/src/resolve.c b/src/resolve.c
> > index d3b483a8..993bbf34 100644
> > --- a/src/resolve.c
> > +++ b/src/resolve.c
> > @@ -40,6 +40,7 @@
> >   struct resolve_ops {
> >   	void (*set_dns)(struct resolve *resolve, char **dns_list);
> >   	void (*set_domains)(struct resolve *resolve, char
> > **domain_list);
> > +	void (*set_mdns)(struct resolve *resolve, char *mdns);
> 
> should probably be const char *mdns

OK. Shouldn't the nearby `char **` parameters be `const char *const *`
too, then?

> 
> >   	void (*revert)(struct resolve *resolve);
> >   	void (*destroy)(struct resolve *resolve);
> >   };
> > @@ -78,6 +79,17 @@ void resolve_set_domains(struct resolve
> > *resolve, char **domain_list)
> >   	resolve->ops->set_domains(resolve, domain_list);
> >   }
> >   
> > +void resolve_set_mdns(struct resolve *resolve, char *mdns)
> > +{
> > +	if (!mdns)
> > +		return;
> > +
> > +	if (!resolve->ops->set_mdns)
> > +		return;
> > +
> > +	resolve->ops->set_mdns(resolve, mdns);
> > +}
> > +
> >   void resolve_revert(struct resolve *resolve)
> >   {
> >   	if (!resolve->ops->revert)
> > @@ -112,9 +124,10 @@ struct systemd {
> >   	struct resolve super;
> >   };
> >   
> > -static void systemd_link_dns_reply(struct l_dbus_message *message,
> > +static void systemd_link_generic_reply(struct l_dbus_message
> > *message,
> >   								void
> > *user_data)
> >   {
> > +	const char *type = user_data;
> >   	const char *name;
> >   	const char *text;
> >   
> > @@ -123,8 +136,8 @@ static void systemd_link_dns_reply(struct
> > l_dbus_message *message,
> >   
> >   	l_dbus_message_get_error(message, &name, &text);
> >   
> > -	l_error("resolve-systemd: Failed to modify the DNS entries. %s:
> > %s",
> > -								name,
> > text);
> > +	l_error("resolve-systemd: Failed to modify the %s entries. %s:
> > %s",
> > +								type,
> > name, text);
> >   }
> >   
> >   static bool systemd_builder_add_dns(struct l_dbus_message_builder
> > *builder,
> > @@ -205,23 +218,8 @@ static void resolve_systemd_set_dns(struct
> > resolve *resolve, char **dns_list)
> >   	l_dbus_message_builder_finalize(builder);
> >   	l_dbus_message_builder_destroy(builder);
> >   
> > -	l_dbus_send_with_reply(dbus_get_bus(), message,
> > systemd_link_dns_reply,
> > -								NULL,
> > NULL);
> > -}
> > -
> > -static void systemd_set_link_domains_reply(struct l_dbus_message
> > *message,
> > -								void
> > *user_data)
> > -{
> > -	const char *name;
> > -	const char *text;
> > -
> > -	if (!l_dbus_message_is_error(message))
> > -		return;
> > -
> > -	l_dbus_message_get_error(message, &name, &text);
> > -
> > -	l_error("resolve-systemd: Failed to modify the domain entries.
> > %s: %s",
> > -								name,
> > text);
> > +	l_dbus_send_with_reply(dbus_get_bus(), message,
> > systemd_link_generic_reply,
> > +								"DNS",
> > NULL);
> >   }
> >   
> >   static void resolve_systemd_set_domains(struct resolve *resolve,
> > @@ -266,8 +264,43 @@ static void resolve_systemd_set_domains(struct
> > resolve *resolve,
> >   	l_dbus_message_builder_finalize(builder);
> >   	l_dbus_message_builder_destroy(builder);
> >   
> > -	l_dbus_send_with_reply(dbus_get_bus(), message,
> > -				systemd_set_link_domains_reply, NULL,
> > NULL);
> > +	l_dbus_send_with_reply(dbus_get_bus(), message,
> > systemd_link_generic_reply,
> > +				"domains", NULL);
> > +}
> > +
> > +static void resolve_systemd_set_mdns(struct resolve *resolve, char
> > *mdns)
> > +{
> > +	struct l_dbus_message_builder *builder;
> > +	struct l_dbus_message *message;
> > +
> > +	l_debug("ifindex: %u", resolve->ifindex);
> > +
> > +	if (L_WARN_ON(!systemd_state.is_ready))
> > +		return;
> > +
> > +	message = l_dbus_message_new_method_call(dbus_get_bus(),
> > +					SYSTEMD_RESOLVED_SERVICE,
> > +					SYSTEMD_RESOLVED_MANAGER_PATH,
> > +					SYSTEMD_RESOLVED_MANAGER_INTERF
> > ACE,
> > +					"SetLinkMulticastDNS");
> > +
> > +	if (!message)
> > +		return;
> > +
> > +	builder = l_dbus_message_builder_new(message);
> > +	if (!builder) {
> > +		l_dbus_message_unref(message);
> > +		return;
> > +	}
> > +
> > +	l_dbus_message_builder_append_basic(builder, 'i', &resolve-
> > >ifindex);
> > +	l_dbus_message_builder_append_basic(builder, 's', mdns);
> > +
> > +	l_dbus_message_builder_finalize(builder);
> > +	l_dbus_message_builder_destroy(builder);
> 
> Using a builder for something this simple is probably overkill.  You
> can replace 
> the last ~12 lines by something like:
> 
> l_dbus_message_set_arguments(message, "is", resolve->ifindex, mdns);

OK.

> 
> > +
> > +	l_dbus_send_with_reply(dbus_get_bus(), message,
> > systemd_link_generic_reply,
> > +								"Multic
> > astDNS", NULL);
> >   }
> >   
> >   static void resolve_systemd_revert(struct resolve *resolve)
> > @@ -288,8 +321,8 @@ static void resolve_systemd_revert(struct
> > resolve *resolve)
> >   		return;
> >   
> >   	l_dbus_message_set_arguments(message, "i", resolve->ifindex);
> > -	l_dbus_send_with_reply(dbus_get_bus(), message,
> > systemd_link_dns_reply,
> > -								NULL,
> > NULL);
> > +	l_dbus_send_with_reply(dbus_get_bus(), message,
> > systemd_link_generic_reply,
> > +								"DNS",
> > NULL);
> >   }
> >   
> >   static void resolve_systemd_destroy(struct resolve *resolve)
> > @@ -302,6 +335,7 @@ static void resolve_systemd_destroy(struct
> > resolve *resolve)
> >   static const struct resolve_ops systemd_ops = {
> >   	.set_dns = resolve_systemd_set_dns,
> >   	.set_domains = resolve_systemd_set_domains,
> > +	.set_mdns = resolve_systemd_set_mdns,
> >   	.revert = resolve_systemd_revert,
> >   	.destroy = resolve_systemd_destroy,
> >   };
> > @@ -482,6 +516,7 @@ static void resolve_resolvconf_destroy(struct
> > resolve *resolve)
> >   static struct resolve_ops resolvconf_ops = {
> >   	.set_dns = resolve_resolvconf_set_dns,
> >   	.set_domains = resolve_resolvconf_set_domains,
> > +	.set_mdns = NULL,
> 
> This should already be NULL, so not needed and can be dropped

OK.

> 
> >   	.revert = resolve_resolvconf_revert,
> >   	.destroy = resolve_resolvconf_destroy,
> >   };
> > diff --git a/src/resolve.h b/src/resolve.h
> > index 64ed15f3..09cd9ac2 100644
> > --- a/src/resolve.h
> > +++ b/src/resolve.h
> > @@ -23,5 +23,6 @@
> >   struct resolve *resolve_new(uint32_t ifindex);
> >   void resolve_set_dns(struct resolve *resolve, char **dns_list);
> >   void resolve_set_domains(struct resolve *resolve, char
> > **domain_list);
> > +void resolve_set_mdns(struct resolve *resolve, char *mdns);
> >   void resolve_revert(struct resolve *resolve);
> >   void resolve_free(struct resolve *resolve);
> > 

Thanks for the review, I will send an updated patch.

> 
> Regards,
> -Denis

[PATCH v2] src: configure systemd-resolved's MulticastDNS= setting

[Daniel Lin]

[-- Attachment #1: Type: text/plain, Size: 6601 bytes --]

When using iwd.conf:[General].EnableNetworkConfiguration=true, it is not
possible to configure systemd.network:[Network].MulticastDNS= as
systemd-networkd considers the link to be unmanaged. This patch allows
iwd to configure that setting on systemd-resolved directly.
---
 src/iwd.config.rst |  7 +++++
 src/netconfig.c    |  6 ++++
 src/resolve.c      | 70 ++++++++++++++++++++++++++++++----------------
 src/resolve.h      |  1 +
 4 files changed, 60 insertions(+), 24 deletions(-)

diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 0064dfd2..d430909a 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -212,6 +212,13 @@ The group ``[Network]`` contains network configuration related settings.
 
        If not specified, ``300`` is used as default.
 
+   * - MulticastDNS
+     - Values: true, **false**, resolve
+
+       Configures multicast DNS. See ``man 5 systemd.network`` for details.
+
+       Only applies when ``NameResolvingService=systemd``.
+
 Blacklist
 ---------
 
diff --git a/src/netconfig.c b/src/netconfig.c
index cf092b78..749ba0ad 100644
--- a/src/netconfig.c
+++ b/src/netconfig.c
@@ -963,6 +963,8 @@ bool netconfig_configure(struct netconfig *netconfig,
 				const uint8_t *mac_address,
 				netconfig_notify_func_t notify, void *user_data)
 {
+	char *mdns;
+
 	netconfig->dns4_overrides = l_settings_get_string_list(active_settings,
 							"IPv4", "DNS", ' ');
 
@@ -1006,6 +1008,10 @@ bool netconfig_configure(struct netconfig *netconfig,
 
 	netconfig_ipv6_select_and_install(netconfig);
 
+	mdns = l_settings_get_string(active_settings, "Network", "MulticastDNS");
+	resolve_set_mdns(netconfig->resolve, mdns);
+	l_free(mdns);
+
 	return true;
 }
 
diff --git a/src/resolve.c b/src/resolve.c
index d3b483a8..5b20b8c9 100644
--- a/src/resolve.c
+++ b/src/resolve.c
@@ -40,6 +40,7 @@
 struct resolve_ops {
 	void (*set_dns)(struct resolve *resolve, char **dns_list);
 	void (*set_domains)(struct resolve *resolve, char **domain_list);
+	void (*set_mdns)(struct resolve *resolve, const char *mdns);
 	void (*revert)(struct resolve *resolve);
 	void (*destroy)(struct resolve *resolve);
 };
@@ -78,6 +79,17 @@ void resolve_set_domains(struct resolve *resolve, char **domain_list)
 	resolve->ops->set_domains(resolve, domain_list);
 }
 
+void resolve_set_mdns(struct resolve *resolve, const char *mdns)
+{
+	if (!mdns)
+		return;
+
+	if (!resolve->ops->set_mdns)
+		return;
+
+	resolve->ops->set_mdns(resolve, mdns);
+}
+
 void resolve_revert(struct resolve *resolve)
 {
 	if (!resolve->ops->revert)
@@ -112,9 +124,10 @@ struct systemd {
 	struct resolve super;
 };
 
-static void systemd_link_dns_reply(struct l_dbus_message *message,
+static void systemd_link_generic_reply(struct l_dbus_message *message,
 								void *user_data)
 {
+	const char *type = user_data;
 	const char *name;
 	const char *text;
 
@@ -123,8 +136,8 @@ static void systemd_link_dns_reply(struct l_dbus_message *message,
 
 	l_dbus_message_get_error(message, &name, &text);
 
-	l_error("resolve-systemd: Failed to modify the DNS entries. %s: %s",
-								name, text);
+	l_error("resolve-systemd: Failed to modify the %s entries. %s: %s",
+								type, name, text);
 }
 
 static bool systemd_builder_add_dns(struct l_dbus_message_builder *builder,
@@ -205,23 +218,8 @@ static void resolve_systemd_set_dns(struct resolve *resolve, char **dns_list)
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
-	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
-								NULL, NULL);
-}
-
-static void systemd_set_link_domains_reply(struct l_dbus_message *message,
-								void *user_data)
-{
-	const char *name;
-	const char *text;
-
-	if (!l_dbus_message_is_error(message))
-		return;
-
-	l_dbus_message_get_error(message, &name, &text);
-
-	l_error("resolve-systemd: Failed to modify the domain entries. %s: %s",
-								name, text);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"DNS", NULL);
 }
 
 static void resolve_systemd_set_domains(struct resolve *resolve,
@@ -266,8 +264,31 @@ static void resolve_systemd_set_domains(struct resolve *resolve,
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
-	l_dbus_send_with_reply(dbus_get_bus(), message,
-				systemd_set_link_domains_reply, NULL, NULL);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+				"domains", NULL);
+}
+
+static void resolve_systemd_set_mdns(struct resolve *resolve, const char *mdns)
+{
+	struct l_dbus_message *message;
+
+	l_debug("ifindex: %u", resolve->ifindex);
+
+	if (L_WARN_ON(!systemd_state.is_ready))
+		return;
+
+	message = l_dbus_message_new_method_call(dbus_get_bus(),
+					SYSTEMD_RESOLVED_SERVICE,
+					SYSTEMD_RESOLVED_MANAGER_PATH,
+					SYSTEMD_RESOLVED_MANAGER_INTERFACE,
+					"SetLinkMulticastDNS");
+
+	if (!message)
+		return;
+
+	l_dbus_message_set_arguments(message, "is", resolve->ifindex, mdns);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"MulticastDNS", NULL);
 }
 
 static void resolve_systemd_revert(struct resolve *resolve)
@@ -288,8 +309,8 @@ static void resolve_systemd_revert(struct resolve *resolve)
 		return;
 
 	l_dbus_message_set_arguments(message, "i", resolve->ifindex);
-	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
-								NULL, NULL);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"DNS", NULL);
 }
 
 static void resolve_systemd_destroy(struct resolve *resolve)
@@ -302,6 +323,7 @@ static void resolve_systemd_destroy(struct resolve *resolve)
 static const struct resolve_ops systemd_ops = {
 	.set_dns = resolve_systemd_set_dns,
 	.set_domains = resolve_systemd_set_domains,
+	.set_mdns = resolve_systemd_set_mdns,
 	.revert = resolve_systemd_revert,
 	.destroy = resolve_systemd_destroy,
 };
diff --git a/src/resolve.h b/src/resolve.h
index 64ed15f3..2301a288 100644
--- a/src/resolve.h
+++ b/src/resolve.h
@@ -23,5 +23,6 @@
 struct resolve *resolve_new(uint32_t ifindex);
 void resolve_set_dns(struct resolve *resolve, char **dns_list);
 void resolve_set_domains(struct resolve *resolve, char **domain_list);
+void resolve_set_mdns(struct resolve *resolve, const char *mdns);
 void resolve_revert(struct resolve *resolve);
 void resolve_free(struct resolve *resolve);
-- 
2.30.1

Re: [PATCH] src: configure systemd-resolved's MulticastDNS= setting

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

Hi Daniel,

>> Is there a default?  If so, it should be in bold.  What happens if
>> this setting
>> is omitted?
> 
> If the setting is omitted, we don't touch the default value. Which

Ok, so there's no default that iwd should pick.

> appears to be false, but somebody could patch systemd-resolved to
> change that, so I wasn't sure if that should be documented here. I'll
> add that though.

So instead we may want to mention that if this setting is omitted, the 
systemd-networkd default is used.

>>> +	mdns = l_settings_get_string(active_settings, "Network",
>>> "MulticastDNS");
>>
>> This can return NULL if no MulticastDNS setting is found.  What
>> happens then?
> 
> Nothing happens, there's a null check in resolve_set_mdns().
> 

k, makes sense.   I somehow missed that originally.

>>> @@ -40,6 +40,7 @@
>>>    struct resolve_ops {
>>>    	void (*set_dns)(struct resolve *resolve, char **dns_list);
>>>    	void (*set_domains)(struct resolve *resolve, char
>>> **domain_list);
>>> +	void (*set_mdns)(struct resolve *resolve, char *mdns);
>>
>> should probably be const char *mdns
> 
> OK. Shouldn't the nearby `char **` parameters be `const char *const *`
> too, then?
> 

Possibly.  I think we resisted using the const variation since not many old C 
hacks like to use such syntax.  So we kept using 'char **'.

For passing simple C-strings we like to be const-correct though.

Regards,
-Denis

Re: [PATCH v2] src: configure systemd-resolved's MulticastDNS= setting

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

On 2/11/21 12:11 PM, Daniel Lin wrote:
> When using iwd.conf:[General].EnableNetworkConfiguration=true, it is not
> possible to configure systemd.network:[Network].MulticastDNS= as
> systemd-networkd considers the link to be unmanaged. This patch allows
> iwd to configure that setting on systemd-resolved directly.
> ---
>   src/iwd.config.rst |  7 +++++
>   src/netconfig.c    |  6 ++++
>   src/resolve.c      | 70 ++++++++++++++++++++++++++++++----------------
>   src/resolve.h      |  1 +
>   4 files changed, 60 insertions(+), 24 deletions(-)
> 

LGTM, only nitpick is..

> diff --git a/src/iwd.config.rst b/src/iwd.config.rst
> index 0064dfd2..d430909a 100644
> --- a/src/iwd.config.rst
> +++ b/src/iwd.config.rst
> @@ -212,6 +212,13 @@ The group ``[Network]`` contains network configuration related settings.
>   
>          If not specified, ``300`` is used as default.
>   
> +   * - MulticastDNS
> +     - Values: true, **false**, resolve

As discussed in the v1 reply, we should not indicate a default and mention that 
this setting isn't set at all if omitted.

> +
> +       Configures multicast DNS. See ``man 5 systemd.network`` for details.
> +
> +       Only applies when ``NameResolvingService=systemd``.
> +
>   Blacklist
>   ---------
>   

Regards,
-Denis

[PATCH v3] src: configure systemd-resolved's MulticastDNS= setting

[Daniel Lin]

[-- Attachment #1: Type: text/plain, Size: 6708 bytes --]

When using iwd.conf:[General].EnableNetworkConfiguration=true, it is not
possible to configure systemd.network:[Network].MulticastDNS= as
systemd-networkd considers the link to be unmanaged. This patch allows
iwd to configure that setting on systemd-resolved directly.
---
 src/iwd.config.rst |  9 ++++++
 src/netconfig.c    |  6 ++++
 src/resolve.c      | 70 ++++++++++++++++++++++++++++++----------------
 src/resolve.h      |  1 +
 4 files changed, 62 insertions(+), 24 deletions(-)

diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 0064dfd2..b3c9491e 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -212,6 +212,15 @@ The group ``[Network]`` contains network configuration related settings.
 
        If not specified, ``300`` is used as default.
 
+   * - MulticastDNS
+     - Values: true, false, resolve
+
+       Configures multicast DNS on each interface. If not specified,
+       systemd-resolved's default value will remain untouched.
+       See ``man 5 systemd.network`` for details.
+
+       Only applies when ``NameResolvingService=systemd``.
+
 Blacklist
 ---------
 
diff --git a/src/netconfig.c b/src/netconfig.c
index cf092b78..749ba0ad 100644
--- a/src/netconfig.c
+++ b/src/netconfig.c
@@ -963,6 +963,8 @@ bool netconfig_configure(struct netconfig *netconfig,
 				const uint8_t *mac_address,
 				netconfig_notify_func_t notify, void *user_data)
 {
+	char *mdns;
+
 	netconfig->dns4_overrides = l_settings_get_string_list(active_settings,
 							"IPv4", "DNS", ' ');
 
@@ -1006,6 +1008,10 @@ bool netconfig_configure(struct netconfig *netconfig,
 
 	netconfig_ipv6_select_and_install(netconfig);
 
+	mdns = l_settings_get_string(active_settings, "Network", "MulticastDNS");
+	resolve_set_mdns(netconfig->resolve, mdns);
+	l_free(mdns);
+
 	return true;
 }
 
diff --git a/src/resolve.c b/src/resolve.c
index d3b483a8..5b20b8c9 100644
--- a/src/resolve.c
+++ b/src/resolve.c
@@ -40,6 +40,7 @@
 struct resolve_ops {
 	void (*set_dns)(struct resolve *resolve, char **dns_list);
 	void (*set_domains)(struct resolve *resolve, char **domain_list);
+	void (*set_mdns)(struct resolve *resolve, const char *mdns);
 	void (*revert)(struct resolve *resolve);
 	void (*destroy)(struct resolve *resolve);
 };
@@ -78,6 +79,17 @@ void resolve_set_domains(struct resolve *resolve, char **domain_list)
 	resolve->ops->set_domains(resolve, domain_list);
 }
 
+void resolve_set_mdns(struct resolve *resolve, const char *mdns)
+{
+	if (!mdns)
+		return;
+
+	if (!resolve->ops->set_mdns)
+		return;
+
+	resolve->ops->set_mdns(resolve, mdns);
+}
+
 void resolve_revert(struct resolve *resolve)
 {
 	if (!resolve->ops->revert)
@@ -112,9 +124,10 @@ struct systemd {
 	struct resolve super;
 };
 
-static void systemd_link_dns_reply(struct l_dbus_message *message,
+static void systemd_link_generic_reply(struct l_dbus_message *message,
 								void *user_data)
 {
+	const char *type = user_data;
 	const char *name;
 	const char *text;
 
@@ -123,8 +136,8 @@ static void systemd_link_dns_reply(struct l_dbus_message *message,
 
 	l_dbus_message_get_error(message, &name, &text);
 
-	l_error("resolve-systemd: Failed to modify the DNS entries. %s: %s",
-								name, text);
+	l_error("resolve-systemd: Failed to modify the %s entries. %s: %s",
+								type, name, text);
 }
 
 static bool systemd_builder_add_dns(struct l_dbus_message_builder *builder,
@@ -205,23 +218,8 @@ static void resolve_systemd_set_dns(struct resolve *resolve, char **dns_list)
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
-	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
-								NULL, NULL);
-}
-
-static void systemd_set_link_domains_reply(struct l_dbus_message *message,
-								void *user_data)
-{
-	const char *name;
-	const char *text;
-
-	if (!l_dbus_message_is_error(message))
-		return;
-
-	l_dbus_message_get_error(message, &name, &text);
-
-	l_error("resolve-systemd: Failed to modify the domain entries. %s: %s",
-								name, text);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"DNS", NULL);
 }
 
 static void resolve_systemd_set_domains(struct resolve *resolve,
@@ -266,8 +264,31 @@ static void resolve_systemd_set_domains(struct resolve *resolve,
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
-	l_dbus_send_with_reply(dbus_get_bus(), message,
-				systemd_set_link_domains_reply, NULL, NULL);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+				"domains", NULL);
+}
+
+static void resolve_systemd_set_mdns(struct resolve *resolve, const char *mdns)
+{
+	struct l_dbus_message *message;
+
+	l_debug("ifindex: %u", resolve->ifindex);
+
+	if (L_WARN_ON(!systemd_state.is_ready))
+		return;
+
+	message = l_dbus_message_new_method_call(dbus_get_bus(),
+					SYSTEMD_RESOLVED_SERVICE,
+					SYSTEMD_RESOLVED_MANAGER_PATH,
+					SYSTEMD_RESOLVED_MANAGER_INTERFACE,
+					"SetLinkMulticastDNS");
+
+	if (!message)
+		return;
+
+	l_dbus_message_set_arguments(message, "is", resolve->ifindex, mdns);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"MulticastDNS", NULL);
 }
 
 static void resolve_systemd_revert(struct resolve *resolve)
@@ -288,8 +309,8 @@ static void resolve_systemd_revert(struct resolve *resolve)
 		return;
 
 	l_dbus_message_set_arguments(message, "i", resolve->ifindex);
-	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_dns_reply,
-								NULL, NULL);
+	l_dbus_send_with_reply(dbus_get_bus(), message, systemd_link_generic_reply,
+								"DNS", NULL);
 }
 
 static void resolve_systemd_destroy(struct resolve *resolve)
@@ -302,6 +323,7 @@ static void resolve_systemd_destroy(struct resolve *resolve)
 static const struct resolve_ops systemd_ops = {
 	.set_dns = resolve_systemd_set_dns,
 	.set_domains = resolve_systemd_set_domains,
+	.set_mdns = resolve_systemd_set_mdns,
 	.revert = resolve_systemd_revert,
 	.destroy = resolve_systemd_destroy,
 };
diff --git a/src/resolve.h b/src/resolve.h
index 64ed15f3..2301a288 100644
--- a/src/resolve.h
+++ b/src/resolve.h
@@ -23,5 +23,6 @@
 struct resolve *resolve_new(uint32_t ifindex);
 void resolve_set_dns(struct resolve *resolve, char **dns_list);
 void resolve_set_domains(struct resolve *resolve, char **domain_list);
+void resolve_set_mdns(struct resolve *resolve, const char *mdns);
 void resolve_revert(struct resolve *resolve);
 void resolve_free(struct resolve *resolve);
-- 
2.30.1

Re: [PATCH v3] src: configure systemd-resolved's MulticastDNS= setting

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

Hi Daniel,

On 2/11/21 2:54 PM, Daniel Lin wrote:
> When using iwd.conf:[General].EnableNetworkConfiguration=true, it is not
> possible to configure systemd.network:[Network].MulticastDNS= as
> systemd-networkd considers the link to be unmanaged. This patch allows
> iwd to configure that setting on systemd-resolved directly.
> ---
>   src/iwd.config.rst |  9 ++++++
>   src/netconfig.c    |  6 ++++
>   src/resolve.c      | 70 ++++++++++++++++++++++++++++++----------------
>   src/resolve.h      |  1 +
>   4 files changed, 62 insertions(+), 24 deletions(-)
> 

I tweaked the line breaks in the patch slightly to fit on 80 char columns.

Applied, thanks.

Regards,
-Denis