Other dns-related bugs:

- https://bugzilla.gnome.org/show_bug.cgi?id=695383#c3
  asks for a default-configuration for dns.

- https://bugzilla.redhat.com/show_bug.cgi?id=1228707
  ask for a possibility to prefer static dns-configuration over auto.

Having a D-Bus API to modify configuration values directly is a possibility, but having a special "SetDnsOptions" API might also be fine.

But I really want to see NMConfig as implementation of how to store/write our configuration.

Let me come up with a patch to NMConfig to implement writing of options.

Added API to NMconfig to write global configuration values. See bug 750558.

I like the idea of having a generic API to set options in NMConfig.

(In reply to Thomas Haller from comment #1)
> Other dns-related bugs:
> 
> - https://bugzilla.gnome.org/show_bug.cgi?id=695383#c3
>   asks for a default-configuration for dns.
> 
> - https://bugzilla.redhat.com/show_bug.cgi?id=1228707
>   ask for a possibility to prefer static dns-configuration over auto.

As I understand it, when global DNS options are specified, they would
override any connection specific setting; so this, together with an
additional per-connection option as 'ignore-global-dns=yes/NO',
should cover the use case of bug #695383.

rh #1228707 is somehow related and asks for a way to change the
priority of static (per-connection) DNS servers over DHCP-supplied
ones. At the moment when ipv4.method is Auto, static DNS servers are
appended after servers obtained from DHCP; a new 'prefer-static-dns'
option would allow to reverse the priority and put static ones in
front. However, if 'ignore-auto-dns' is enabled, only static DNS
servers will be used, independently of the value of
'prefer-static-dns'. And of course if there are global DNS options and
'ignore-global-dns' is 'no', global information will be used instead.


About the keyfile format, how about something like:

  [global-dns]
  # (enable=yes/NO) can be inferred by the presence of [global-dns] section
  searches=foo.com,bar.org
  options=debug,timeout:5

  # A section for every domain, name must start with "global-dns-domain"
  [global-dns-domain-0]
  domain=company.com
  servers=1.1.1.1,2.2.2.2
  options=opt1,opt2

  [global-dns-domain-default]
  domain=*
  servers=3.3.3.3
  options=opt3

(In reply to Beniamino Galvani from comment #4)

> About the keyfile format, how about something like:
> 
>   [global-dns]
>   # (enable=yes/NO) can be inferred by the presence of [global-dns] section
>   searches=foo.com,bar.org
>   options=debug,timeout:5


I updated the write support to NMConfig (th/nm-config-intern-bgo750558).

It allows basically to write and read ~one~ global keyfile to 
  /var/lib/NetworkManager/NetworkManager-intern.conf

For one it can contain sections with a prefix [.intern*]. These sections are completely separate from what the user can configure via "NetworkManager.conf".
Effectively, that translates your example to:

  [.intern.global-dns]
  [.intern.global-dns-domain-0]
  [.intern.global-dns-domain-default]


It also supports overwriting values from NetworkManager.conf. That should work well if you have one individual value that you set or unset (like keyfile.hostname).
It works badly, if you have a set of inter-related options, because the write-API only merges values one-by-one, because it does not understand the interrelation between multiple (dns-options) settings.


All I am saying is that if this configuration does not overlap with what the user can specify in NetworkManager.conf, then all is easy. That means, it can only be modified internally (e.g. D-Bus API).
If we want to support to merge the options from NetworkManager.conf and our [.intern] sections, then we can implement it. But this merging cannot happen automatically, instead we must carefully look at both sides and do the right thing.


For example, if user had:

  [global-dns-domain-0]
  domain=company.com
  servers=1.1.1.1,2.2.2.2
  options=opt1,opt2

and modifies it via afterwards D-Bus, we could add an internal section like:

  [.intern.global-dns-domain-0]
  match-user-conf=domain:company.com,servers:1.1.1.1\,2.2.2.2,options:opt1\,opt2
  domain=company2.com
  servers=2.3.4.3
  options=opt3

On a restart of NM, we would check if our internal section's 'match-user-conf' still equals to what the user originally had, and reject it otherwise.

(In reply to Thomas Haller from comment #5)
> (In reply to Beniamino Galvani from comment #4)
> 
> 
> For example, if user had:
> 
>   [global-dns-domain-0]
>   domain=company.com
>   servers=1.1.1.1,2.2.2.2
>   options=opt1,opt2
> 
> and modifies it via afterwards D-Bus, we could add an internal section like:
> 
>   [.intern.global-dns-domain-0]
>  
> match-user-conf=domain:company.com,servers:1.1.1.1\,2.2.2.2,options:opt1\,
> opt2
>   domain=company2.com
>   servers=2.3.4.3
>   options=opt3
> 
> On a restart of NM, we would check if our internal section's
> 'match-user-conf' still equals to what the user originally had, and reject
> it otherwise.

I like that, it's quite simple actually. Let me come up with a patch to NMConfig. It would only be of concern for NMConfig, other parts would not have to care.

It means such sections are set atomically, either from user (NM.conf) or from internal (D-Bus).

Then we have:

   - internal only sections in [.intern*]
   - atomic values (like for keyfile.hostname)
   - atomic sections (like for [global-dns*]).


With that we only have to do proper logic to merge individual sections... which should be simple if we define the sections properly.

(In reply to Thomas Haller from comment #6)
> (In reply to Thomas Haller from comment #5)
> I like that, it's quite simple actually. Let me come up with a patch to
> NMConfig. It would only be of concern for NMConfig, other parts would not
> have to care.
> 
> It means such sections are set atomically, either from user (NM.conf) or
> from internal (D-Bus).
> 
> Then we have:
> 
>    - internal only sections in [.intern*]
>    - atomic values (like for keyfile.hostname)
>    - atomic sections (like for [global-dns*]).
> 
> 
> With that we only have to do proper logic to merge individual sections...
> which should be simple if we define the sections properly.

implemented at bug 750558.

(In reply to Thomas Haller from comment #6)
> It means such sections are set atomically, either from user (NM.conf) or
> from internal (D-Bus).
>
> Then we have:
>
>    - internal only sections in [.intern*]
>    - atomic values (like for keyfile.hostname)
>    - atomic sections (like for [global-dns*]).
>
>
> With that we only have to do proper logic to merge individual sections...
> which should be simple if we define the sections properly.

As discussed on IRC, the problem I see with this approach and the
keyfile format described above is that sections names for global DNS
domains are arbitrary and we use section name to match internal sections
to user sections. So we could end up having:

 # Set by user
 [global-dns-domain-0]
 domain=*
 ...

 # Set through D-Bus
 [.intern.global-dns-domain-default]
 domain=*
 ...

In this situation the merge logic would create 2 sections having the
same domain, but this is wrong IMO.

The alternative we talked about is to have a single
[global-dns-domains] section and describe domains as, for example:

 domain.0.match=*
 domain.0.servers=1.1.1.1
 domain.1.match=example.com
 domain.1.servers=2.2.2.2

Changes to the section would be atomic, in the sense that a change
done by user on a single parameter for a domain would invalidate all
the domains in the internal configuration.

(In reply to Beniamino Galvani from comment #8)
> (In reply to Thomas Haller from comment #6)
> > It means such sections are set atomically, either from user (NM.conf) or
> > from internal (D-Bus).
> >
> > Then we have:
> >
> >    - internal only sections in [.intern*]
> >    - atomic values (like for keyfile.hostname)
> >    - atomic sections (like for [global-dns*]).
> >
> >
> > With that we only have to do proper logic to merge individual sections...
> > which should be simple if we define the sections properly.
> 
> As discussed on IRC, the problem I see with this approach and the
> keyfile format described above is that sections names for global DNS
> domains are arbitrary and we use section name to match internal sections
> to user sections. So we could end up having:
> 
>  # Set by user
>  [global-dns-domain-0]
>  domain=*
>  ...
> 
>  # Set through D-Bus
>  [.intern.global-dns-domain-default]
>  domain=*

As the NMConfig API is currently, having a section [.intern.*] means those sections never overlap with what the user can configure. We can configure all our D-Bus provided settings in [.intern*] sections, separate from the user configured sections.

That however doesn't yet tell us how to merge the user and internal parts of the configuration. In this case the NMConfig API leaves it entirely up to the caller to get that right -- which is the actual problem at hand.



Alternatively, I added *atomic sections* to the write API (yeay for the name :) ). It means, from NMConfig API we overwrite entire sections of the user configuration.

  # Set by user, but actually hidden.
  [global-dns-domain-0]
  domain=*
  some-key=a

and we would overwrite it (entirely) with another section:

  # overwrite through D-Bus
  [global-dns-domain-0]
  domain=something.else
  some-other-key=b
  

Now, if the user modifies its configuration, the API would detect that, and basically reject our internal overwrite, so that the most recent user modification wins.

That is already nice. But it doesn't tell us, how we merge conflicting sections as in your example:

Suppose we start with a clean state:

 # Set through D-Bus
 [global-dns-domain-default]
 domain=*
 ...

No conflict yet.


Now user edits:

 # Set by user
 [global-dns-domain-0]
 domain=*
 ...
 # Set through D-Bus
 [global-dns-domain-default]
 domain=*
 ...

Now we have a conflict, but we just reject our D-Bus section -- because the user provided configuration has preference here.

Now, we update again the config via D-Bus. In this case, we have two options to resolve the conflict:


a) Just no longer use our previous section, and overwrite the user-provided section.

 # Set by user
 [global-dns-domain-0]
 domain=*
 ...

 # Overwrite via D-Bus
 [global-dns-domain-0]
 domain=*
 ...

 # remove other previous internal section
 #[global-dns-domain-default]
 #domain=*



How does that sound? I think its reasonably well doable

*** Bug 751200 has been marked as a duplicate of this bug. ***

First implementation pushed to branch bg/global-dns-conf-bgo750458, based on th/nm-config-intern-bgo750558.

>> libnm-core: make strv/slist conversion functions available to NM

the commit message is wrong. Mixes "from" and "to".



>> core: add global DNS configuration to NMConfigData
    
+    group = internal ? GLOB_DNS_INTERN_GROUP : NM_CONFIG_DATA_GLOBAL_DNS_GROUP;
+    domain_prefix = internal ? GLOB_DNS_INTERN_DOM_PREFIX : 
           NM_CONFIG_DATA_GLOBAL_DNS_GROUP;
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^



+#define NM_CONFIG_DATA_GLOBAL_DNS_GROUP      "global-dns"
+#define NM_CONFIG_DATA_GLOBAL_DNS_DOMAIN_PREFIX  "global-dns-domain-"
 
should move to nm-config.h and be named:

#define NM_CONFIG_KEYFILE_GROUP_
#define NM_CONFIG_KEYFILE_GROUPPREFIX_

Also move GLOB_DNS_INTERN_GROUP there.


+NMGlobalDnsConfig *nm_config_data_get_global_dns_config (const NMConfigData *self);
 
return const pointer?


Rename NMGlobalDnsDomainConfig and NMGlobalDnsConfig to have a NMConfig prefix.



+    GHashTable *domains;

are the domains unsorted? Because a hash table looses the order.




>> manager: export DNS global configuration through D-Bus
   
+         settings = nm_auth_chain_get_data (chain, "settings");
+         domains = nm_auth_chain_get_data (chain, "domains");
+         data = nm_config_get_data (priv->config);
+         config = nm_config_data_global_dns_config_from_dbus (data, settings, domains, &error);
+
+         if (!error)
+              keyfile = nm_config_data_global_dns_config_update_keyfile (data, config, &error);
+         if (!error)
+              nm_config_set_values (priv->config, keyfile, TRUE, FALSE);


nm_config_set_values() is a low-level function. I think NMConfig should provide a more convenient function (nm_config_set_dns_config) that does this.

(In reply to Thomas Haller from comment #12)
>
> +    GHashTable *domains;
> 
> are the domains unsorted? Because a hash table looses the order.

Yes, the order doesn't matter and if there are sub-domains of existing entries, the more specific one has the precedence.

Repushed with fixes for all other comments, thanks.

>> manager: export DNS global configuration through D-Bus
    

+         /* it's allowed */
+         GHashTable *settings;
+         GHashTable *domains;
+         NMConfigGlobalDns *dns_config;
+
+         settings = nm_auth_chain_get_data (chain, "settings");
+         domains = nm_auth_chain_get_data (chain, "domains");
+         dns_config = nm_config_data_global_dns_from_dbus (settings, domains, &error);
+
+         if (!error)
+              nm_config_set_global_dns (priv->config, dns_config, &error);


I think this leaks @dns_config.


Again back to another question. Wouldn't a D-Bus property be more suitable then a method (+ changed signal)?

Currently the NM.conf format is like:

[global-dns-domain.*]
domain=x
options..


how about having the domain in the section name?

[global-dns-domain-x]
options=...






there is one advantage to this:

If leave the possibility to later allow the D-Bus API to coexist with manual user-configuration. Because then the we could implement the merging using atomic sections.

Another advantage is, that it doesn't allow the following invalid configuration:

[global-dns-domain-1]
domain=freedesktop.org

[global-dns-domain-2]
domain=freedesktop.org

(In reply to Thomas Haller from comment #14)
> >> manager: export DNS global configuration through D-Bus
> I think this leaks @dns_config.

Good catch, thanks.

> Again back to another question. Wouldn't a D-Bus property be more suitable
> then a method (+ changed signal)?

That was the original plan, but I found some issues implementing the
get_property() method; I don't know if it's a limitation of dbus-glib
marshaling code or if I'm doing something wrong.

The problem is that when the property is defined as "a{sv}" and a
GValue of type G_TYPE_HASH is added to the hash for the "domains" key,
dbus-glib fails when it tries to marshal the property value and returns
error messages like:

  WARNING **: Cannot marshal type "GHashTable" in variant
  CRITICAL **: Internal error: could not marshal type "GHashTable_gchararray+GValue_" in variant

(In reply to Beniamino Galvani from comment #16)
> (In reply to Thomas Haller from comment #14)

> > Again back to another question. Wouldn't a D-Bus property be more suitable
> > then a method (+ changed signal)?
> 
> That was the original plan, but I found some issues implementing the
> get_property() method; I don't know if it's a limitation of dbus-glib
> marshaling code or if I'm doing something wrong.
> 
> The problem is that when the property is defined as "a{sv}" and a
> GValue of type G_TYPE_HASH is added to the hash for the "domains" key,
> dbus-glib fails when it tries to marshal the property value and returns
> error messages like:
> 
>   WARNING **: Cannot marshal type "GHashTable" in variant
>   CRITICAL **: Internal error: could not marshal type
> "GHashTable_gchararray+GValue_" in variant

as this is public API, I think it is worth investing some extra effort to get it right (from an API point-of-view). And IMO a property is more suitable.
Maybe dcbw or danw know how to achieve it with dbus-glib??

You can't use G_TYPE_HASH directly, because dbus-glib doesn't know what the key/value types in the hash table are, because GHashTable doesn't convey that.  Instead, you create a GType with dbus_g_type_get_map() that describes this.

See include/nm-dbus-glib-types.h...  so now what you do is use DBUS_TYPE_G_MAP_OF_VARIANT like so:

void
myfunc(GHashTable *domains_hash, GHashTable *property_hash)
{
GValue *value;

value = g_slice_new0 (GValue);
g_value_init (value, DBUS_TYPE_G_MAP_OF_VARIANT);
g_value_set_boxed (value, <GHashTable pointer>);

g_hash_table_insert (property_hash, "domains", value);
}

this of course  means that you need to ensure your 'property_hash' value destroy function is something like this:

static void
_gvalue_destroy (gpointer data)
{
	GValue *value = (GValue *) data;

	g_value_unset (value);
	g_slice_free (GValue, value);
}

which means you probably need to duplicate all the GValues you stuff into property_hash.

See https://git.gnome.org/browse/network-manager-vpnc/tree/src/nm-vpnc-service-vpnc-helper.c for a pretty simple example of all this.

Note that nm-vpnc-service-vpnc-helper.c has some oddities though:

1) it doesn't actually create the hash table specifying a value destroy function, so the GValues added to it are leaked.  But since it's a short-lived process that doesn't matter.

2) it uses "dbus_g_type_get_map ("GHashTable", G_TYPE_STRING, G_TYPE_VALUE)" instead of DBUS_TYPE_G_MAP_OF_VARIANT because the VPN plugins can't include nm-dbus-glib-types.h that I talked about above.  But the result is the same since DBUS_TYPE_G_MAP_OF_VARIANT is just a macro that does that same thing

(In reply to Thomas Haller from comment #15)
> Currently the NM.conf format is like:
> 
> [global-dns-domain.*]
> domain=x
> options..
> 
> 
> how about having the domain in the section name?
> 
> [global-dns-domain-x]
> options=...
> 
> 
> 
> 
> 
> 
> there is one advantage to this:
> 
> If leave the possibility to later allow the D-Bus API to coexist with manual
> user-configuration. Because then the we could implement the merging using
> atomic sections.
> 
> Another advantage is, that it doesn't allow the following invalid
> configuration:
> 
> [global-dns-domain-1]
> domain=freedesktop.org
> 
> [global-dns-domain-2]
> domain=freedesktop.org

Yeah, I like this.

Would be great to have some testscases for reading/writing the global dns config stuff too.

(In reply to Dan Williams from comment #18)
> You can't use G_TYPE_HASH directly, because dbus-glib doesn't know what the
> key/value types in the hash table are, because GHashTable doesn't convey
> that.  Instead, you create a GType with dbus_g_type_get_map() that describes
> this.

Yeah, now it's working, thanks!

(In reply to Thomas Haller from comment #17)
> as this is public API, I think it is worth investing some extra effort to
> get it right (from an API point-of-view). And IMO a property is more
> suitable.
> Maybe dcbw or danw know how to achieve it with dbus-glib??

A property seems a better choice from an API perspective, and now with
Dan's suggestion I managed to implement the property marshaling for
nested hashes.

There are still some implementation details to figure out, but they
shouldn't be a big problem; for example, there isn't a simple way of
returning an error to the Set() method when the DNS configuration is
already set by user configuration.

Maybe this check can be added to the D-Bus connection filter in
NMManager together with the permission check (it seems that also the
permission check requires some changes as now it only supports boolean
properties).

Fixed according to above comments and repushed to bg/global-dns-conf-bgo750458.

> core: add D-Bus conversion helpers

So, I forgot about licensing issues.  This code is BSD but NM is GPLv2+; I don't think we want to start mixing things up.

But here's the best part: I took this code from NM originally and contributed it to wpa_supplicant back in 2006.  So we could reach back into old NM releases and find the last time that code was there (probably right before we switched to dbus-glib around 0.6 or 0.7) and copy that code, rather than using BSD-licensed stuff.  The code should be substantially the same, plus it'll already use glib :)

The other patch looks OK to me.

(In reply to Dan Williams from comment #25)
> But here's the best part: I took this code from NM originally and
> contributed it to wpa_supplicant back in 2006.  So we could reach back into
> old NM releases and find the last time that code was there (probably right
> before we switched to dbus-glib around 0.6 or 0.7) and copy that code,
> rather than using BSD-licensed stuff.  The code should be substantially the
> same, plus it'll already use glib :)

Now I've updated the file from old NM sources and changed the copyright notice.
I had to introduce some changes because the original code didn't support nested dictionaries, and thus now the parsing function doesn't return an iterator but returns directly an array containing all the entries of the dictionary (which can in turn contain other dictionaries). The function basically returns the full dictionary tree in a single invocation.

+    <property name="GlobalDnsConfiguration" type="a{sv}" access="readwrite">
+      <tp:docstring>
+        Dictionary of global DNS settings where the key is one of
+        "searches", "options" and "domains".  "searches" and "options"
+        are string arrays describing the list of search domains and
+        resolver options.  "domains" is a dictionary mapping domain
+        names to dictionaries with keys "servers" and "options".
+        "servers" is a string list of DNS servers, "options" is a
+        string list of domain-specific options.
+      </tp:docstring>
+    </property>

How about:

Dictionary of global DNS settings where the key is one of "searches", "options" and "domains".  The values for the "searches" and "options" keys are string arrays describing the list of search domains and resolver options, respectively.  The value of the "domains" key is a second-level dictionary, where each key is a domain name, and each key's value is a third-level dictionary with the keys "servers" and "options". "servers" is a string list of DNS servers, "options" is a string list of domain-specific options.

--------

Stef; does this API look OK?  Basically:

org.freedesktop.DBus.Properties.Set("org.freedesktop.NetworkManager",
                                    "GlobalDnsConfiguration",
{
    searches: [ baz.foobar.com, foobar.com ],
    options: [ timeout:3, rotate ],
    domains: {
        "foobar.com": {
            servers: [ 1.2.3.4, 1.2.3.5 ]
        }
        "*": {
            servers: [ 8.8.8.8 ]
        }
    }
});

and NM would use this to override anything that got passed back from DHCP, WWAN, PPoE, SLAAC, etc.

I say lets fix up the documentation, and commit the branch and we can get Stef's feedback once he's back from PTO.

>> core: add support for reading global DNS configuration from keyfile

I dislike a bit that NMConfigGlobalDns is a public type and directly exposed. That allows a wrong usage, which modifies the content of NMConfigData (NMConfigData is supposed to be immutable).
But OK, probably it's too much effort to get some encapsulation... just don't modify it...

Not sure, but after gdbus porting, don't we export the setting as GVariant? Maybe it makes sense to store it internally as GVariant too -- because that one is indeed immutable.


>> manager: export DNS global configuration D-Bus property

+ nm_log_err (LOGD_CORE, "set global DNS failed with error: %s", error->message);

this will trigger error-logs from invalid arguments via D-Bus. I think it's wrong to allow a user to trigger logging messages of this kind, although the user is priviledged). DEBUG seems enough.


nm_config_data_global_dns_from_dbus():

+    /* An empty value clears and disables the internal configuration */
+    empty = !dns_config->searches && !dns_config->options && +
+    if (!empty && !g_hash_table_lookup (dns_config->domains, "*")) {
+         g_set_error_literal (error, 1, 0, "Global DNS configuration is +         return NULL;
+    }

this check if different then what load_global_dns() does. I think both should agree to what constitutes a valid setting.





+         while (g_hash_table_iter_next (&iter, &k, &v)) {
+              domain = global_dns_domain_from_dbus ((char *) k, (GValue *) v);
+              if (domain)
+                   g_hash_table_insert (dns_config->domains, g_strdup ((char *) k), domain);
+         }

global_dns_domain_from_dbus() has an unused parameter @name. At the same time, it is not ensured that @k != "" -- which would be invalid.



Pushed minor 2 fixup commits.

(In reply to Thomas Haller from comment #29)
> >> core: add support for reading global DNS configuration from keyfile
>
> Not sure, but after gdbus porting, don't we export the setting as GVariant?

Yes, the setting is exported as a GVariant now.

> Maybe it makes sense to store it internally as GVariant too -- because that
> one is indeed immutable.

What do you mean with internally? The configuration is a
NMConfigGlobalDns structure and I think it should remain as is.

> >> manager: export DNS global configuration D-Bus property
>
> + nm_log_err (LOGD_CORE, "set global DNS failed with error: %s",
> error->message);
>
> this will trigger error-logs from invalid arguments via D-Bus. I think it's
> wrong to allow a user to trigger logging messages of this kind, although the
> user is priviledged). DEBUG seems enough.

Fixed.

> nm_config_data_global_dns_from_dbus():

> this check if different then what load_global_dns() does. I think both
> should agree to what constitutes a valid setting.

In both places there's a check for the presence of the "*" domain,
which must always exist in a valid configuration.

However, nm_config_data_global_dns_from_dbus() accepts also a
completely empty configuration with the meaning "clear the internal
configuration".

This is the only difference between the two and I think it's
reasonable to keep it.

> global_dns_domain_from_dbus() has an unused parameter @name. At the same
> time, it is not ensured that @k != "" -- which would be invalid.

Fixed now.

> Pushed minor 2 fixup commits.

Thanks, branch updated and rebased on master.

I'm very excited that this change has been merged into master, but I'm curious about one feature I've been looking for related to this.  Perhaps this is the wrong bug ticket, but I've looked at a few related ones and I think it's the right one.  I apologize if I'm intruding and should put this somewhere else.

The feature I'm looking for is the ability not to *override* certain DNS settings that NetworkManager finds and configures, but to append additional information.

I read through the bug ticket and it's not clear if this will support, for example, appending two additional domains to dns-search, so that you have the dns-search setting provided by the DHCP server, but also can append.

In other words, I'd find it wonderful if NetworkManager supported the behavior akin to dhclient's prepend/append statements (see man dhclient.conf for more details).  It's clear from the ticket traffic here that you'll support the equivalent of dhclient's "supersede", but will this change also support "prepend/append"?

Thanks for your time and for your hard work improving NetworkManager!

(In reply to Bradley M. Kuhn from comment #31)
> I'm very excited that this change has been merged into master, but I'm
> curious about one feature I've been looking for related to this.

This hasn't been merged yet, but I hope to do it soon.

> The feature I'm looking for is the ability not to *override* certain DNS
> settings that NetworkManager finds and configures, but to append additional
> information.

Currently on master the static values (for instance, dns-searches) are
appended to the ones provided through DHCP. Then there is the
ipv4.ignore-auto-dns flag which discards the DHCP-provided DNS values
when set. At the moment there isn't any way to change the order of
DHCP/static DNS information and put the static values first, but we
already have a downstream bug requesting this:

https://bugzilla.redhat.com/show_bug.cgi?id=1228707

After the global DNS branch gets merged, it will be possible to set a
global DNS configuration valid for all the connections, which will
replace both static and dynamic settings. So it will not support the
use of global configuration together with other (DHCP or static)
settings.

Branch bg/global-dns-conf-bgo750458 rebased on current master.

+    case PROP_GLOBAL_DNS_CONFIGURATION:
+         config_data = nm_config_get_data (priv->config);
+         dns_config = nm_config_data_get_global_dns (config_data);
+         nm_config_data_global_dns_config_to_dbus (dns_config, value);


PROP_GLOBAL_DNS_CONFIGURATION being a property is invoked often. E.g. every nmcli invocation will read the property during libnm initialization (I think???).

Could we cache the returned GVariant? After all, GVariants are conveniently immutable and ref-counted. Similarly, NMConfigData is immutable and ref-counted. Just have NMConfigData create the GVariant once, cache it and then something like...
Or have NMConfigGlobalDns cache it a proper getter... (doesn't really matter who does it).




+         g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE_BOOLEAN));
+         ptr = GINT_TO_POINTER (g_variant_get_boolean (value));

+    g_object_set (object, pfd->glib_propname, ptr, NULL);

When setting a gboolean property, the argument must be (gboolean, gint). You cannot pass a pointer there.



+    if (!g_strcmp0 (pfd->glib_propname, NM_MANAGER_GLOBAL_DNS_CONFIGURATION)) {

strcmp? If there is no glib_propname set, we *want* a serious failure.


»···»···»···reply = g_dbus_message_new_method_error (pfd->message,
»···»···»···»···»···»···»···»···»···»···»···»···»··· NM_PERM_DENIED_ERROR,
»···»···»···»···»···»···»···»···»···»···»···»···»··· (error_message = "Global DNS configuration already set by user"));

white space.




The /* do some extra type checking... */ block should move before 
  args = g_dbus_message_get_body (pfd->message);
First validation of the object, and then dive more into the details.


(error_message = "Global DNS configuration already set by user"));

"... already set via configuration file"?




»···»···global_dns = nm_config_data_get_global_dns (NM_CONFIG_GET_DATA);

nm-manager should not access the singleton getter directly, but use of priv->config (for which it owns a reference).



»···priv->global_dns = load_global_dns (priv->keyfile_user, FALSE);
»···if (!priv->global_dns)
»···»···priv->global_dns = load_global_dns (priv->keyfile_intern, TRUE);

if the user configured settings via D-Bus (and they are presistated in keyfile_intern), and later configures them in /etc, the intern settings are not cleared.
That means:
  - user configures setting-A via D-Bus
  - user configures setting-B via /etc
  - user deletes setting-B from /etc
then setting-A reappears. I think that is wrong. Once we load setting-B (which hides setting-A), setting-A should be removed from the internal keyfile so that it cannot re-appear.

All you need to achive that, is modify intern_config_read() to
  - ignore internal parts, if /etc dns-settings are present
  - set @out_needs_rewrite in that case.





I quite dislike that we expose a struct NMConfigDataGlobalDns, which is part of the immutable NMConfigData object. Can we not hide the internals to make the API immutable? (ok, it is arguably "const" already)

All that needs is:

const char *const* nm_config_global_dns_get_searches(self);
const char *const* nm_config_global_dns_get_options(self);
gboolean nm_config_global_dns_is_internal(self);
const NMConfigGlobalDnsDomain *const* nm_config_global_dns_get_domains(self);


and have
 typedef struct {
+    char *domain;
     GSList *servers;
     GSList *options;
 } NMConfigGlobalDnsDomain;


Ok, one might argue that a exposed "const NMConfigGlobalDnsDomain" is just as (im)mutable as "const NMConfigGlobalDns *"... so... but

  domain = g_hash_table_lookup(global, "*");
  server = domain->servers->data;

already involves casting twice a void pointer... so, the API doesn't really encourage proper use.

  


I personally prefer strv lists over GList (and IMO they can be conveniently iterated).
But for add_global_config() it seems that
+    g_hash_table_iter_init (&iter, config->domains);
+    while (g_hash_table_iter_next (&iter, &key, &value)) {
has no stable order. I think the order might be important here, and exposing it as const NMConfigGlobalDnsDomain *const* would solve that neatly.

similarly, the sort order for compute_hash() should be in the same order. But maybe the better order is the order of appearance, over strcmp?

+         keys = g_list_sort (g_hash_table_get_keys (global->domains), (GCompareFunc) strcmp);
+         for (key = keys; key; key = g_list_next (key)) {






How about adding a:
    if (global)
        nm_config_global_dns_hash(global, sum);
for compute_hash()?

(In reply to Thomas Haller from comment #34)
> PROP_GLOBAL_DNS_CONFIGURATION being a property is invoked often. E.g. every
> nmcli invocation will read the property during libnm initialization (I
> think???).

> Could we cache the returned GVariant? After all, GVariants are conveniently
> immutable and ref-counted. Similarly, NMConfigData is immutable and
> ref-counted. Just have NMConfigData create the GVariant once, cache it and
> then something like...

Actually it's only invoked only when requested directly through D-Bus
and this is not implemented in libnm yet, so in my opinion the caching
is not so useful at the moment; but yes, this could be an improvement.

> Once we load setting-B
> (which hides setting-A), setting-A should be removed from the internal
> keyfile so that it cannot re-appear.

Makes sense, fixed.

> I quite dislike that we expose a struct NMConfigDataGlobalDns, which is part
> of the immutable NMConfigData object. Can we not hide the internals to make
> the API immutable? (ok, it is arguably "const" already)

I changed the global DNS config and domain types to be opaque and
added accessor methods, what do you think of this approach?

> But for add_global_config() it seems that
> +    g_hash_table_iter_init (&iter, config->domains);
> +    while (g_hash_table_iter_next (&iter, &key, &value)) {
> has no stable order. I think the order might be important here, and exposing
> it as const NMConfigGlobalDnsDomain *const* would solve that neatly.

I think that the order in which domains are evaluated is not important
and that's why the property is exposed on D-Bus as a dictionary. Using
an array would not match the D-Bus representation and the order of
that array would be stable but would potentially change between
different D-Bus SetProperty calls.

> How about adding a:
>     if (global)
>         nm_config_global_dns_hash(global, sum);
> for compute_hash()?

Done and fixed also other points above.

LGTM.

Merged to master:

f04b27bd1f8d core: merge branch 'bg/global-dns-conf-bgo750458'