Add mDNS support for DNS backends which support them. #35

ipuustin · 2017-11-01T14:18:29Z

Add a new setting mdns to connections. This setting can be used for enabling support for mDNS resolving and mDNS host registration for a given interface. Also add a function (update_mdns) to DNS plugin interface and implement this function for systemd-resolved plugin.

So, in other words, if a connection has mdns=1, enable mDNS support in systemd-resolved for the interface that the connection uses.

ipuustin · 2017-11-02T14:10:31Z

Rebased...

thom311 · 2017-11-06T15:51:10Z

libnm-core/nm-setting-connection.c

+	 */
+	g_object_class_install_property
+		(object_class, PROP_MDNS,
+		 g_param_spec_enum (NM_SETTING_CONNECTION_MDNS, "", "",


We often don't add properties of enum type, because those cannot be extended without breaking backward compatibility.
Only use g_param_spec_enum(), if you are sure that no other enum values will be added.
Otherwise, look for example at "NMSettingIP6Config:addr-gen-mode"

thom311 · 2017-11-06T15:54:47Z

clients/common/nm-meta-setting-desc.c

+	PROPERTY_INFO_WITH_DESC (NM_SETTING_CONNECTION_MDNS,
+		.property_type = DEFINE_PROPERTY_TYPE (
+			.get_fcn =                  _get_fcn_connection_mdns,
+			.set_fcn =                  _set_fcn_connection_mdns,


Don't re-implement the get/set functions, unless necessary.

As this is an enum, use

.property_type = &_pt_gobject_enum,

(possibly also setting .property_typ_data appropriately).

thom311 · 2017-11-06T15:57:07Z

libnm-core/nm-setting-connection.c

+		                    NM_TYPE_MDNS,
+		                    NM_MDNS_UNKNOWN,
+		                    G_PARAM_READWRITE |
+		                    NM_SETTING_PARAM_REAPPLY_IMMEDIATELY |


It's not NM_SETTING_PARAM_REAPPLY_IMMEDIATELY

thom311 · 2017-11-06T15:57:55Z

libnm-core/nm-setting-connection.h

 NMMetered   nm_setting_connection_get_metered (NMSettingConnection *setting);
 NM_AVAILABLE_IN_1_2
 NMSettingConnectionLldp nm_setting_connection_get_lldp (NMSettingConnection *setting);
+NMMdns   nm_setting_connection_get_mdns (NMSettingConnection *setting);


Needs NM_AVAILABLE_IN_1_12 (note, that we just missed 1.10 release).

thom311 · 2017-11-06T15:58:13Z

libnm-core/nm-setting-connection.c

+	 *
+	 * This feature requires a plugin which supports mDNS. One such
+	 * plugin is dns-systemd-resolved.
+	 **/


Needs Since: 1.12

thom311 · 2017-11-06T15:58:36Z

libnm-core/nm-dbus-interface.h

+ * @NM_MDNS_YES:         Use mDNS for hostname registration and resolving
+ * @NM_MDNS_NO:          Disable mDNS
+ * @NM_MDNS_RESOLVE:     Use mDNS only for resolving
+ **/


Since: 1.12

thom311 · 2017-11-06T16:00:10Z

libnm/libnm.ver

 	nm_setting_connection_autoconnect_slaves_get_type;
 	nm_setting_connection_get_autoconnect_slaves;
 	nm_setting_connection_get_lldp;
+	nm_setting_connection_get_mdns;


As 1.10-rc1 is already released, we better don't change API anymore (not even extend)

These new entries, must move to a new linker section libnm_1_12_0

thom311 · 2017-11-06T16:01:15Z

libnm-core/nm-setting-connection.c

+	 **/
+	/* ---ifcfg-rh---
+	 * property: mdns
+	 * variable: CONNECTION_MDNS(+)


You didn't implement this flag in ifcfg-rh reader/writer, but it's kinda important to do (yeah, that sucks).

thom311 · 2017-11-06T16:03:25Z

libnm-core/nm-setting-connection.c

 		                   G_PARAM_STATIC_STRINGS));
+	/**
+	 * NMSettingConnection:mdns:
+	 *


If you build with --enable-gtk-doc, you'll see that this comment ends up in ./clients/common/settings-docs.c file (this also breaks make check).

You need to copy the generated file over to ./clients/common/settings-docs.c.in, and commit the changes as well.

thom311 · 2017-11-06T16:27:55Z

src/nm-policy.c

+	gs_free NMSettingsConnection **connections = NULL;
+
+	connections = nm_settings_get_connections_sorted (priv->settings, NULL);
+


this is not right. You iterate over all connection-profiles, without regard whether they are currently active or not.
also, nm_setting_connection_get_interface_name() is not required to be present.

I would merge this commit in the previous one. But it seems to me, this needs to be re-thought.

Note that for regular DNS settings, the manager tracks it for all devices at the same time. Meaning, whenever a device's configuration changes, we propagate the change to the NMDnsManager via nm_dns_manager_add_ip6_config(). But note that that is not only done during update_ip6_dns() (which gets called when a different device gets the best default-route), it happens every time something on the device changed.
On the other hand, your update_mdns() iterates over all connections, setting nm_dns_manager_set_mdns() for each ifindex it finds. That seems wrong. Instead, pass the mdns configuration for every device during nm_dns_manager_add_ip6_config()

ipuustin · 2017-11-10T13:15:47Z

@thom311: Thanks for the review! The code base is new to me, and I really appreciate the help. I did the changes you requested, but some for some things (such as the policy changes) I'm still not sure if I got your meaning right.

thom311 · 2017-11-14T10:35:40Z

Hi. I pushed a few suggestions to https://github.com/NetworkManager/NetworkManager/commits/th/pr/35 for your consideration.

The second commit is the non-trivial one. I think the logic there is not right. update_ip_dns() is called when the "best-device" changes. Then, in your code we would find the current best-device, and call nm_dns_manager_set_mdns() for its ifindex/mdns setting. I think that's wrong, because you want to set nm_dns_manager_set_mdns() for every device which is currently active. Also, when the device deactivates, you need to reset the mdns setting for the ifindex (currently the code will only configure mdns on the next best device).

It's not entirely clear to me how it should be, but what I tried to say was that NMDnsManager already tracks for each device some configuration. See nm_dns_manager_add_ip_config() and nm_dns_manager_remove_ip_config(). One problem is, that it tracks the configuration by ifname. That is not very suitable, because devices can be renamed. The ifindex would be the correct identifier for a device instead. Of course, you chould hack it, to use the ifname in nm_dns_manager_set_mdns(dnsmgr, ifname, mdns) (and in the layers below resolve the ifindex, before calling "SetLinkMulticastDNS"), but that seems ugly.
Also, while NMDnsManager tracks the per-device configs (IPv4 and IPv6), it's currently not very suitable to be extended for your new mdns setting. You could hack around that, by putting the mdns setting into NMIP4Config/NMIP6Config, but that seems ugly. A better solution would be to refactor NMDnsManager, to be able to track various per-device data (ifname, NMIP4Config, NMIP6Config, mdns) by ifindex. Doing that is a bit of an effort however...

@bengal, what do you think?

bengal · 2017-11-14T13:16:57Z

I think that as a first step we could add functions modeled as the existing API:

 nm_dns_manager_add_mdns_config (NMDnsManager, ifname, NMSettingConnectionMdns)
 nm_dns_manager_remove_mdns_config (NMDnsManager, ifname)

The DNS manager would store this information and update the plugin
when we call update_dns().

Later, it would be nice to do what Thomas suggests and change the API to be more generic and use the ifindex.

ipuustin · 2017-11-16T13:22:46Z

Ok, I refactored the connection settings to be separate from the ip settings in both dns manager and policy. @thom311, I incorporated your fixup! patches (those which were still relevant after the refactoring), I hope that was your intention?

thom311 · 2017-11-16T13:25:07Z

@thom311, I incorporated your fixup! patches (those which were still relevant after the refactoring), I hope that was your intention?

Yes, it was. Thanks you!!

ipuustin · 2017-11-17T11:27:10Z

Now the ifindex information should be carried to the DNS manager. Also updating ifindex for a given NMdevice is supported.

thom311 · 2017-11-21T15:43:08Z

Now the ifindex information should be carried to the DNS manager. Also updating ifindex for a given
NMdevice is supported.

I think it goes to the right direction (thanks!!), but could we change it a bit more?

You currently track two distinct entries

NMDnsIPConfigData, in priv->ip_configs, for NMIP6Config and NMIP4Config.
NMDnsConnectionConfigData in priv->connection_configs for mdns setting.

I already think that it's wrong that for one ifindex/ifname, we have possibly two NMDnsIPConfigData instances.

It think NMDnsConnectionConfigData and NMDnsIPConfigData should get merged in one, to have fields ifname, ifindex, ip4_config, ip6_config, mdns. Then, each ifindex could have zero or one such data. The data might have only parts of the settings present (e.g. ip4_config could be set, but ip6_config not. Or maybe ip4_config set but mdns not -- in which case, you might need two flags mdns_value and mdns_present to indicate whether the value is set at all).

I would only have one master function to set the data:

    typedef enum {
        NM_DNS_MANAGER_CONFIG_MODE_IGNORE,
        NM_DNS_MANAGER_CONFIG_MODE_SET,
        NM_DNS_MANAGER_CONFIG_MODE_UNSET,
    } NMDnsManagerConfigMode; 
    nm_dns_manager_config_set (NMDnsManager *dns_manager,
                               int ifindex,
                               NMDnsManagerConfigMode ifname_mode,
                               const char *ifname,
                               NMDnsManagerConfigMode mdns_mode,
                               gboolean mdns,
                               NMDnsManagerConfigMode ip4_config_mode,
                               NMIP4Config *ip4_config,
                               NMDnsManagerConfigMode ip6_config_mode,
                               NMIP4Config *ip6_config);

a function like nm_dns_manager_add_connection_config would be just a shortcut like

    void nm_dns_manager_add_ip_config (NMDnsManager *dns_manager, 
                                       int ifindex,
                                       NMIPConfig *ip_config)
   {
        if (NM_IS_IP4_CONFIG (ip_config)) {
            nm_dns_manager_config_set (dns_manager,
                                       ifindex,
                                       NM_DNS_MANAGER_CONFIG_MODE_IGNORE,
                                       NULL,
                                       NM_DNS_MANAGER_CONFIG_MODE_IGNORE,
                                       FALSE,
                                       NM_DNS_MANAGER_CONFIG_MODE_SET,
                                       ip_config,
                                       NM_DNS_MANAGER_CONFIG_MODE_IGNORE,
                                       NULL);
        } else {
                // SAME
        }
    }

I think that's how it should look like eventually. If you would like to work on that, that would be great. Otherwise, I think what you did so far could be the base for that change, and we could do the changes on top your patches.

bengal · 2017-11-22T08:41:51Z

The branch looks fine to me now; I agree with Thomas that we can merge it now and improve it later, or we can wait for you to do the suggested changes. Thanks!

bengal · 2017-11-22T08:50:28Z

There is one failure in 'make check', can you fix it? It's enough to add the new setting to the array in libnm-core/tests/test-general.c - test_connection_diff_a_only()

ipuustin · 2017-11-24T08:07:09Z

I fixed the check failure and the compilation warning. I think it might be a good idea to merge now and improve later, because refactoring the API has a lot to do with project style preferences and not that much to do with mDNS itself. :-)

bengal · 2017-12-12T13:12:35Z

LGTM. @thom311, what do you think?

Add support for mDNS as a connection-level property. Update ifcfg-rh and keyfile plugins to support it.

Update nm-policy.c and nm-dns-manager.c so that the connection-specific settings get propagated to DNS manger. Currently the only such value is the mDNS status. Add update_mdns() function to DNS plugin interface. If a DNS plugin supports mDNS, it can set an interface with a given index to support mDNS resolving or also register the current hostname. The mDNS support is currently added only to systemd-resolved DNS plugin.

ipuustin · 2017-12-14T10:39:55Z

Rebased to fix the libnm.ver conflict.

thom311 · 2017-12-20T16:24:34Z

I took the patches, incorporated them in #44, and expanded on them.
I think this pull request should be closed and discussion should continue at #44.

Closing this pull request, but please reopen if need be. Thanks.

ipuustin · 2017-12-22T13:07:27Z

Ok, thanks!

When NM is restarted and a new object-manager is created, ensure that signal handlers are disconnected from the old one. Fixes the following: assertion failed: (object_manager == priv->object_manager) #0 __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 __GI_abort () at abort.c:90 #2 g_assertion_message (domain=domain@entry=0x7fcac0b845ff "libnm", file=file@entry=0x7fcac0b84c95 "libnm/nm-client.c", line=line@entry=2506, func=func@entry=0x7fcac0b863a0 <__func__.34881> "name_owner_changed", message=message@entry=0x7fcac20b05f0 "assertion failed: (object_manager == priv->object_manager)") at gtestutils.c:2429 #3 g_assertion_message_expr (domain=domain@entry=0x7fcac0b845ff "libnm", file=file@entry=0x7fcac0b84c95 "libnm/nm-client.c", line=line@entry=2506, func=func@entry=0x7fcac0b863a0 <__func__.34881> "name_owner_changed", expr=expr@entry=0x7fcac0b856a0 "object_manager == priv->object_manager") at gtestutils.c:2444 #4 name_owner_changed (object=<optimized out>, pspec=<optimized out>, user_data=0x7fcac204e480) at libnm/nm-client.c:2506 #8 <emit signal notify:name-owner on instance 0x7fcac2053c60 [GDBusObjectManagerClient]> (instance=instance@entry=0x7fcac2053c60, signal_id=<optimized out>, detail=<optimized out>) at gsignal.c:3439 #5 g_closure_invoke (closure=0x7fcac20af390, return_value=return_value@entry=0x0, n_param_values=2, param_values=param_values@entry=0x7ffde58d9ec0, invocation_hint=invocation_hint@entry=0x7ffde58d9e60) at gclosure.c:801 #6 signal_emit_unlocked_R (node=node@entry=0x7fcac2052090, detail=detail@entry=185, instance=instance@entry=0x7fcac2053c60, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffde58d9ec0) at gsignal.c:3627 #7 g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffde58da050) at gsignal.c:3383 #9 g_object_dispatch_properties_changed (object=0x7fcac2053c60 [GDBusObjectManagerClient], n_pspecs=<optimized out>, pspecs=<optimized out>) at gobject.c:1061 #10 g_object_notify (pspec=<optimized out>, object=0x7fcac2053c60 [GDBusObjectManagerClient]) at gobject.c:1155 #11 g_object_notify (object=object@entry=0x7fcac2053c60 [GDBusObjectManagerClient], property_name=property_name@entry=0x7fcabe9d2b29 "name-owner") at gobject.c:1202 #12 on_notify_g_name_owner (object=<optimized out>, pspec=<optimized out>, user_data=0x7fcac2053c60) at gdbusobjectmanagerclient.c:1262 #16 <emit signal notify:g-name-owner on instance 0x7fcaa8004440 [GDBusProxy]> (instance=instance@entry=0x7fcaa8004440, signal_id=<optimized out>, detail=<optimized out>) at gsignal.c:3439 #13 g_closure_invoke (closure=0x7fcaa80194f0, return_value=return_value@entry=0x0, n_param_values=2, param_values=param_values@entry=0x7ffde58da370, invocation_hint=invocation_hint@entry=0x7ffde58da310) at gclosure.c:801 #14 signal_emit_unlocked_R (node=node@entry=0x7fcac2052090, detail=detail@entry=299, instance=instance@entry=0x7fcaa8004440, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffde58da370) at gsignal.c:3627 #15 g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffde58da500) at gsignal.c:3383 #17 g_object_dispatch_properties_changed (object=0x7fcaa8004440 [GDBusProxy], n_pspecs=<optimized out>, pspecs=<optimized out>) at gobject.c:1061 #18 g_object_notify (pspec=<optimized out>, object=0x7fcaa8004440 [GDBusProxy]) at gobject.c:1155 #19 g_object_notify (object=object@entry=0x7fcaa8004440 [GDBusProxy], property_name=property_name@entry=0x7fcabe9d2b27 "g-name-owner") at gobject.c:1202 #20 on_name_owner_changed (connection=<optimized out>, sender_name=<optimized out>, object_path=<optimized out>, interface_name=<optimized out>, signal_name=<optimized out>, parameters=<optimized out>, user_data=0x7fcaa8015b50) at gdbusproxy.c:1353 #21 emit_signal_instance_in_idle_cb (data=0x7fcaa40ff400) at gdbusconnection.c:3701 #22 g_main_context_dispatch (context=0x7fcac204eea0) at gmain.c:3152 #23 g_main_context_dispatch (context=context@entry=0x7fcac204eea0) at gmain.c:3767 #24 g_main_context_iterate (context=context@entry=0x7fcac204eea0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3838 #25 g_main_context_iteration (context=0x7fcac204eea0, context@entry=0x0, may_block=may_block@entry=1) at gmain.c:3899 #26 nmc_readline_helper (prompt=prompt@entry=0x7fcac2087f60 "The connection is not saved. Do you really want to quit? (yes/no) [no] ") at clients/cli/common.c:986 #27 nmc_readline (prompt_fmt=<optimized out>) at clients/cli/common.c:1055 #28 confirm_quit () at clients/cli/connections.c:6459 #29 do_connection_edit (connection_type=<optimized out>, connection=0x7fcac208eca0, nmc=0x7fcac12a3a60 <nm_cli>) at clients/cli/connections.c:7611 #30 do_connection_edit (nmc=0x7fcac12a3a60 <nm_cli>, argc=1, argv=0x7ffde58db0b0) at clients/cli/connections.c:7948 #31 call_cmd (nmc=0x7fcac12a3a60 <nm_cli>, simple=0x7fcac2052490 [GSimpleAsyncResult], cmd=0x7fcac1291ae0 <connection_cmds+128>, argc=2, argv=0x7ffde58db0a8) at clients/cli/common.c:1315 #32 got_client (source_object=<optimized out>, res=<optimized out>, user_data=0x7fcac2051830) at clients/cli/common.c:1297 #33 g_simple_async_result_complete (simple=0x7fcac2052500 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #34 client_inited (source=0x7fcac204e480 [NMClient], result=0x7fcac20565f0, user_data=0x7fcac2052500) at libnm/nm-client.c:1839 #35 g_simple_async_result_complete (simple=0x7fcac20565f0 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #36 init_async_complete (init_data=init_data@entry=0x7fcac2046820) at libnm/nm-client.c:2339 #37 async_inited_obj_nm (init_data=0x7fcac2046820) at libnm/nm-client.c:2337 #38 async_inited_obj_nm (object=0x7fcac2073080 [NMRemoteConnection], result=0x7fcac2089ed0, user_data=0x7fcac2046820) at libnm/nm-client.c:2357 #39 g_simple_async_result_complete (simple=0x7fcac2089ed0 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #40 init_async_parent_inited (error=0x0, init_data=0x7fcaa408a0f0) at libnm/nm-remote-connection.c:677 #41 init_async_parent_inited (source=<optimized out>, result=<optimized out>, user_data=0x7fcaa408a0f0) at libnm/nm-remote-connection.c:689 #42 g_simple_async_result_complete (simple=0x7fcac2089c30 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #43 complete_in_idle_cb (data=<optimized out>) at gsimpleasyncresult.c:813 #44 g_main_context_dispatch (context=0x7fcac204eea0) at gmain.c:3152 #45 g_main_context_dispatch (context=context@entry=0x7fcac204eea0) at gmain.c:3767 #46 g_main_context_iterate (context=0x7fcac204eea0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3838 #47 g_main_loop_run (loop=0x7fcac2045ab0) at gmain.c:4032 #48 main (argc=<optimized out>, argv=<optimized out>) at clients/cli/nmcli.c:642 https://bugzilla.redhat.com/show_bug.cgi?id=1471245 (cherry picked from commit 7758071)

ipuustin force-pushed the mdns branch from d2079b5 to be398b5 Compare November 2, 2017 14:09

thom311 requested changes Nov 6, 2017

View reviewed changes

ipuustin force-pushed the mdns branch from be398b5 to 50dc127 Compare November 10, 2017 13:09

ipuustin force-pushed the mdns branch from 50dc127 to b3d3068 Compare November 16, 2017 13:19

ipuustin force-pushed the mdns branch 2 times, most recently from 3ff94a7 to d2e0b93 Compare November 17, 2017 11:25

ipuustin force-pushed the mdns branch from d2e0b93 to ccda515 Compare November 17, 2017 11:32

ipuustin force-pushed the mdns branch 2 times, most recently from 51d5526 to 79ea681 Compare November 23, 2017 10:11

ipuustin added 2 commits December 14, 2017 11:15

mdns: add new connection property.

5569322

Add support for mDNS as a connection-level property. Update ifcfg-rh and keyfile plugins to support it.

ipuustin force-pushed the mdns branch from 79ea681 to 1bdd3f5 Compare December 14, 2017 10:38

thom311 mentioned this pull request Dec 20, 2017

Rework NMDnsManager and add MDNS support for systemd-resolved (obsoletes #35) #44

Closed

thom311 closed this Dec 20, 2017

		gs_free NMSettingsConnection **connections = NULL;

		connections = nm_settings_get_connections_sorted (priv->settings, NULL);

Add mDNS support for DNS backends which support them. #35

Add mDNS support for DNS backends which support them. #35

Uh oh!

Conversation

ipuustin commented Nov 1, 2017

Uh oh!

ipuustin commented Nov 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thom311 Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ipuustin commented Nov 10, 2017

Uh oh!

thom311 commented Nov 14, 2017

Uh oh!

bengal commented Nov 14, 2017

Uh oh!

ipuustin commented Nov 16, 2017

Uh oh!

thom311 commented Nov 16, 2017

Uh oh!

ipuustin commented Nov 17, 2017

Uh oh!

thom311 commented Nov 21, 2017

Uh oh!

bengal commented Nov 22, 2017

Uh oh!

bengal commented Nov 22, 2017

Uh oh!

ipuustin commented Nov 24, 2017

Uh oh!

bengal commented Dec 12, 2017

Uh oh!

ipuustin commented Dec 14, 2017

Uh oh!

thom311 commented Dec 20, 2017

Uh oh!

ipuustin commented Dec 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

thom311 Nov 6, 2017 •

edited

Loading