Created attachment 301511 [details] [review]
[PATCH] dns: always write a private resolv.conf to runtime directory

LGTM

(CC: pavlix)

Maybe we can make it a bit simpler too, by not falling back to touching resolv.conf when other methods fail.  If something has enabled resolvconf support, then if resolvconf fails I'm not sure NM should try to work around that, because that shows a deeper problem in the system.  Same for netconfig...

So maybe something like:

	if (update) {
#ifdef RESOLVCONF_PATH
		success = dispatch_resolvconf (searches, nameservers, error);
#elif NETCONFIG_PATH
		success = dispatch_netconfig (searches, nameservers,
		                              nis_domain, nis_servers, error);
#else
		success = update_resolv_conf (searches, nameservers, error, TRUE);
#endif
	}

	if (!success) {
		/* Only update private resolv.conf in NMRUNDIR, ignore errors */
		update_resolv_conf (searches, nameservers, error, FALSE);
	}

But all in all, looks good as it is now.

(In reply to Dan Williams from comment #3)
> Maybe we can make it a bit simpler too, by not falling back to touching
> resolv.conf when other methods fail.  If something has enabled resolvconf
> support, then if resolvconf fails I'm not sure NM should try to work around
> that, because that shows a deeper problem in the system.  Same for
> netconfig...

+1 for not falling back. I think we could eventually remove resolvconf/netconfig support into plugins.

(In reply to Pavel Simerda from comment #4)
> +1 for not falling back. I think we could eventually remove
> resolvconf/netconfig support into plugins.

Note that currently you can use the dnsmasq plugin along with resolvconf/netconfig support.

I had a side-conversation with bgalvani on this point, and the approach he suggested and I thought sounded OK was to add another config option for this.  So you'd have:

dns=[none, dnsmasq, unbound, etc]
resolvconf=[none, resolvconf, netconfig, etc, empty means "write /etc/resolv.conf"]
(or rcmanager?  or resconf? or?)

dns=none == resolvconf=none; eg if one is 'none' the other must be 'none' to preserve dns=none backwards compat.

At config time, a valid RESOLVCONF_PATH would default resolvconf=resolvconf, while a valid NETCONFIG_PATH woudl default resolvconf=netconfig.  Thus you could override your distro compile defaults with config files too.

I thought long and hard about allowing dnsmasq to be used with resolvconf and netconfig, and finally concluded that yes, that's something I think we should still support.  First reason is backwards compat, to ensure we don't break existing installs on upgrade.  The second reason is that you'd loose split DNS with dnsmasq configured via resolvconf/netconfig, because in that configuration NetworkManager would not be pushing split DNS info to dnsmasq and resolvconf/netconfig can't handle split DNS.

(In reply to Dan Williams from comment #3)
> So maybe something like:
> 
> 	if (update) {
> #ifdef RESOLVCONF_PATH
> 		success = dispatch_resolvconf (searches, nameservers, error);
> #elif NETCONFIG_PATH
> 		success = dispatch_netconfig (searches, nameservers,
> 		                              nis_domain, nis_servers, error);
> #else
> 		success = update_resolv_conf (searches, nameservers, error, TRUE);
> #endif
> 	}

Yeah, this seems reasonable.

> 
> 	if (!success) {

I'm not sure about this condition: shouldn't the private version of
resolv.conf be updated also when other methods succeed?

> 		/* Only update private resolv.conf in NMRUNDIR, ignore errors */
> 		update_resolv_conf (searches, nameservers, error, FALSE);
> 	}

Anyway, I reworked the patch according to the comments and pushed the
changes to branch bg/resolv-conf-bgo747821 since now they are split in
multiple commits.

When the selected method fails, other methods are not tried anymore
and the DNS update procedure returns an error.

There is also a new 'rc-manager' option (I'm not sure it's the best
name, suggestions are welcome) that allows selecting the strategy used
to update resolv.conf. To retain backwards compatibility, the default
value is chosen at build time based on whether resolvconf or netconfig
paths are specified.

> 

There's some blank lines at the bottom of init_resolv_conf_manager() now.

And in config_changed_cb() you could do:

if (!(changes & (NM_CONFIG_CHANGE_DNS_MODE | NM_CONFIG_CHANGE_RC_MANAGER))

Beyond that, additional patches look good to me, lets get at least one more review and opinion on 'rc-manager' vs. 'resolvconf' vs 'resconf-manager' or whatever.  I'm OK with the branch if everyone agrees on the option name.

(In reply to Dan Williams from comment #8)
> > 
> 
> There's some blank lines at the bottom of init_resolv_conf_manager() now.
> 
> And in config_changed_cb() you could do:
> 
> if (!(changes & (NM_CONFIG_CHANGE_DNS_MODE | NM_CONFIG_CHANGE_RC_MANAGER))


or

  if (!NM_FLAG_ANY (changes, NM_CONFIG_CHANGE_DNS_MODE |
                             NM_CONFIG_CHANGE_RC_MANAGER))

(In reply to Thomas Haller from comment #9)
> (In reply to Dan Williams from comment #8)
> > > 
> > 
> > There's some blank lines at the bottom of init_resolv_conf_manager() now.
> > 
> > And in config_changed_cb() you could do:
> > 
> > if (!(changes & (NM_CONFIG_CHANGE_DNS_MODE | NM_CONFIG_CHANGE_RC_MANAGER))
> 
> 
> or
> 
>   if (!NM_FLAG_ANY (changes, NM_CONFIG_CHANGE_DNS_MODE |
>                              NM_CONFIG_CHANGE_RC_MANAGER))

Yeah let's do that.

> There's some blank lines at the bottom of init_resolv_conf_manager() now.

>   if (!NM_FLAG_ANY (changes, NM_CONFIG_CHANGE_DNS_MODE |
>                              NM_CONFIG_CHANGE_RC_MANAGER))

Pushed fixups to branch bg/resolv-conf-bgo747821.

Fixups look good.  I say go ahead and merge it, we can always change 'rc-manager' later and alias the old option to the new one.

Pushed to master:

ad6dbc5 dns: merge branch 'bg/resolv-conf-bgo747821'
b1a81e5 man: document 'rc-manager' option
e573977 dns: allow runtime selection of resolv.conf manager
de0d623 dns: don't fall back to other methods when resolvconf or netconfig fail
5f9d348 dns: always write a private resolv.conf to runtime directory