Rework NMDnsManager and add MDNS support for systemd-resolved (obsoletes #35) #44
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c7e4f5c to
60aa952
Compare
Contributor
|
I wonder if NMSettingConnection is the right place for a mdns Maybe we could add now the new dns setting with the only property 'mdns' I have quickly looked at commits and the approach looks good to me. Can |
60aa952 to
64342f7
Compare
64342f7 to
4dc967f
Compare
4dc967f to
7573719
Compare
Just moving code around, no other changes. Follow a certain prefered order of declarations in source files.
When doing changes that affect multiple source files, it's more convenient to build the parts that have less dependencies first. So, to fix the build failures from the core outward.
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.
Also, keep the internal variable of type int. The only way to set the field is via the GObject property setter. At that point, don't yet cast the integer type to enum.
"UNKNOWN" is not a good name. If you don't set the property in the connection explicitly, it should be "DEFAULT". Also, make "DEFAULT" -1. For one, that ensures that the enum's underlying integer type is signed. Otherwise, it's cumbersome to test "if (mdns >= DEFAULT)" because in case of unsigned types, the compiler will warn about the check always being true. Also, it allows for "NO" to be zero. These are no strong reasons, but I tend to think this is better. Also, don't make the property of NMSettingConnection a CONSTRUCT property. Initialize the default manually in the init function. Also, order the numeric values so that DEFAULT < NO < RESOLVE < YES with YES being largest because it enables *the most*.
7573719 to
4264d5e
Compare
Sometimes, we want to use CList to track a simple data item. But contrary
to GList/GSList, we need to define a structure to hold the data pointer
and the CList member.
Add a generic NMCListElem type that can be used for such simple uses.
Before you ask: why not use GList/GSList? Because even simple operations
like g_list_append() is O(n), which kinda defeats the purpose of having
a doubly linked list.
This code is added to a new header file nm-c-list.h, the reason is that
there is no other good place:
- "nm-utils/c-list.h" is a clone of upstream, it should not deviate.
- "nm-utils/c-list-util.h" contains our utils functions for c-list.h
but should be plain C, independent of glib.
- "nm-utils/nm-shared-utils.h" contains our glib related utilities,
but it should not drag in "c-list.h".
So, "nm-c-list.h" is a utility libray that extends "c-list.h" and
requires glib.
A cmp() implementation, for sorting an array with pointers, where each pointer is an inteter according to GPOINTER_TO_INT(). That cames for example handy, if you have a GHashTable with keys GINT_TO_POINTER(). Then you get the list of keys via g_hash_table_get_keys_as_array() and want to sort them.
Use a GHashTable instead of a GArray to construct the list of @interfaces. Also, use NMCListElem instead of GList. With this, the runtime is O(n*log(n)) instead of O(n^2). I belive, we should take care that all our code has a reasonable runtime complexity, even in common use-cases the number of elements is small. This is not about performace, because likely we expect few entries anyway, and the direct GArray implementation is likely faster in those cases. It's about using the data structure that best suits the access pattern. The log(n) part comes from sorting the keys. I also believe we should always aim for a stable behavior. When sending the D-Bus request to resolved, the order of elements should be in ~some~ defined order.
We had two separate queues, one for "SetLinkDNS" and one for "SetLinkDomains". Merge them into one, and track the operation as part of the new RequestItem structure. A visible change to before is that we now would make all requests per-interface first. Prevously, we would first make all SetLinkDNS requests (for all interfaces) and then all SetLinkDomains requests. It feels more correct to order the requests this way, not by type. The reason to merge is, that we will next get another operation and in the current scheme we would need 3 GQueue instances. While at it, refactor the code to use CList. We now anyway would need a new struct to track the operation, requiring to allocate and free it. Previously, we would only track the GVariant argument as data of the GQueue.
Next commit will unify naming of variables, do a trivial rename first to make the diff smaller.
4264d5e to
7f490af
Compare
Don't track the per-device configuration in NMDnsManager by the ifname, but by the ifindex. We should consistently treat the ifindex as the ID of a link, like kernel does. At the few places where we actually need the ifname, resolve it by looking into the platform cache. That is not necessarily the same as the ifname that is currently tracked by NMDevice, because netdev interfaces can be renamed, and NMDevice updates it's link properties delayed. However, the platform cache has the most recent notion of the correct interface name for an ifindex, so if we ever hit a race here, we do it now more correctly. This also temporarily drops support for mdns. Will be re-added next, but differently.
The connection.mdns setting is a per-connection setting, so one might expect that one activated device can only have one MDNS setting at a time. However, with certain VPN plugins (those that don't have their own IP interface, like libreswan), the VPN configuration is merged into the configuration of the device. So, in this case, there might be multiple settings for one device that must be merged. We already have a mechanism for that. It's NMIP4Config. Let NMIP4Config track this piece of information. Although, stricitly speaking this is not tied to IPv4, the alternative would be to introduce a new object to track such data, which would be a tremendous effort and more complicated then this. Luckily, NMDnsManager and NMDnsPlugin are already equipped to handle multiple NMIPConfig instances per device (IPv4 vs. IPv6, and Device vs. VPN). Also make "connection.mdns" configurable via global defaults in NetworkManager.conf.
7f490af to
a28fb8a
Compare
Collaborator
Author
|
after rework and CI run, merged to master. |
lkundrak
pushed a commit
that referenced
this pull request
Feb 2, 2021
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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I took the patches from #35 and expanded on them. The original patches are applied first, with minimal changes (only rebasing). Follow-up commits are based on them. I propose this pull request to obsolete #35.
After rethinking, I think #35 needs to be solved a bit different, so I reworked
NMDnsManagerto track their data by ifindex (not by ifname), and reworked how the MDNS setting is passed to the DNS plugin.Still totally untested :) Please review.