GNOME Bugzilla – Bug 749286
dns: restore fallback to writing /etc/resolv.conf when resolvconf or netconfig fail
Last modified: 2015-05-26 11:41:02 UTC
After the merge of fix for bug 747821, the method used to write /etc/resolv.conf is specified by a configuration parameter and NM doesn't try other methods if the chosen one has not success. To be on the safe side, it would be better to fall back to the 'none' method if other methods fail, so that when the value of 'rc-manager' is misconfigured because some related packages have been installed/removed, the system continues to work properly.
Created attachment 303294 [details] [review] [PATCH] dns: restore fallback to writing /etc/resolv.conf when other methods fail
The core problem here was that on Debian they compile NM with resolvconf support, but of course resolvconf isn't always installed on the system. So on upgrade of these systems, there would be no explicit rc-manager=resolvconf configuration, resolvconf config would fail, and nothing would fall back to writing /etc/resolv.conf. The patch looks good, but I wonder if we shouldn't *only* do the fallback when the executable (netconfig or resolvconf) cannot be found? eg, if they are run successfully and return an non-zero exit, then no fallback is done. Thoughts?
(In reply to Dan Williams from comment #2) > The patch looks good, but I wonder if we shouldn't *only* do the fallback > when the executable (netconfig or resolvconf) cannot be found? eg, if they > are run successfully and return an non-zero exit, then no fallback is done. > Thoughts? that sounds reasonable to me
Created attachment 303525 [details] [review] [PATCH v2] dns: fall back to writing resolv.conf when other methods are not available
(In reply to Thomas Haller from comment #3) > (In reply to Dan Williams from comment #2) > > The patch looks good, but I wonder if we shouldn't *only* do the fallback > > when the executable (netconfig or resolvconf) cannot be found? eg, if they > > are run successfully and return an non-zero exit, then no fallback is done. > > Thoughts? > > that sounds reasonable to me Yes, seems a good idea; I updated the patch.
Review of attachment 303525 [details] [review]: ::: src/dns-manager/nm-dns-manager.c @@ +118,3 @@ + NM_RC_MAN_PROGRAM_UNAVAILABLE, + NM_RC_MAN_ERROR, +} NMResolvConfManResult; I wouldn't namespace this much at all, since it's private to this file. Instead I'd just use 'SpawnResult' and then SR_SUCCESS, SR_NOTFOUND, and SR_ERROR or something similarly simple. @@ +906,3 @@ + + if (result == NM_RC_MAN_PROGRAM_UNAVAILABLE) { + nm_log_warn (LOGD_DNS, "program not available, writing to resolv.conf"); I think this should be nm_log_dbg(), because in the normal fallback case where resolvconf support is built but resolvconf not installed, this would be printed every time DNS got changed, which could be quite a bit.
Created attachment 303638 [details] [review] [PATCH v3] dns: fall back to writing resolv.conf when other methods are not available
(In reply to Dan Williams from comment #6) > I wouldn't namespace this much at all, since it's private to this file. > Instead I'd just use 'SpawnResult' and then SR_SUCCESS, SR_NOTFOUND, and > SR_ERROR or something similarly simple. Ok. > I think this should be nm_log_dbg(), because in the normal fallback case > where resolvconf support is built but resolvconf not installed, this would > be printed every time DNS got changed, which could be quite a bit. Attached new patch, thanks.
LGTM
Pushed to master as a6f5aeeb28c73c2ee94322016df20212c4dd8f0a.