| Message ID | 20190214121456.4784-2-yszhou4tech@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [OpenWrt-Devel,1/2] dnsmasq: allow using dnsmasq as the sole resolver | expand |
Hi Yousong, On Thu, Feb 14, 2019 at 1:15 PM Yousong Zhou <yszhou4tech@gmail.com> wrote:Y > > Option resolvfile should now affect only how dnsmasq itself will run. > The implicit effect of its specific value on other parts of the system > makes the code unnecessarily hard to follow > > Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com> > --- > package/network/services/dnsmasq/Makefile | 2 +- > package/network/services/dnsmasq/files/dnsmasq.init | 7 +------ > 2 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile > index d51f23e48b..5e83b4f8e5 100644 > --- a/package/network/services/dnsmasq/Makefile > +++ b/package/network/services/dnsmasq/Makefile > @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk > > PKG_NAME:=dnsmasq > PKG_VERSION:=2.80 > -PKG_RELEASE:=8 > +PKG_RELEASE:=9 > > PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.xz > PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq > diff --git a/package/network/services/dnsmasq/files/dnsmasq.init b/package/network/services/dnsmasq/files/dnsmasq.init > index 05c555bd8c..22471c11aa 100644 > --- a/package/network/services/dnsmasq/files/dnsmasq.init > +++ b/package/network/services/dnsmasq/files/dnsmasq.init > @@ -888,7 +888,6 @@ dnsmasq_start() > config_get resolvfile "$cfg" resolvfile > [ -n "$resolvfile" -a ! -e "$resolvfile" ] && touch "$resolvfile" > xappend "--resolv-file=$resolvfile" > - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && localuse=1 By removing this line 127.0.0.1 is not added anymore in /tmp/resolv.conf as localuse is 0 by default; as a result local resolving is broken. Hans > fi > > config_get hostsfile "$cfg" dhcphostsfile > @@ -1039,13 +1038,9 @@ dnsmasq_start() > dnsmasq_stop() > { > local cfg="$1" > - local noresolv resolvfile localuse > + local localuse > > - config_get_bool noresolv "$cfg" noresolv 0 > config_get_bool localuse "$cfg" localuse 0 > - config_get resolvfile "$cfg" "resolvfile" > - > - [ "$noresolv" = 0 -a "$resolvfile" = "/tmp/resolv.conf.auto" ] && localuse=1 > [ "$localuse" -gt 0 ] && ln -sf "/tmp/resolv.conf.auto" /tmp/resolv.conf > > rm -f ${BASEDHCPSTAMPFILE}.${cfg}.*.dhcp > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Mon, 18 Feb 2019 at 04:00, Hans Dedecker <dedeckeh@gmail.com> wrote: > > Hi Yousong, > On Thu, Feb 14, 2019 at 1:15 PM Yousong Zhou <yszhou4tech@gmail.com> wrote:Y > > > > Option resolvfile should now affect only how dnsmasq itself will run. > > The implicit effect of its specific value on other parts of the system > > makes the code unnecessarily hard to follow > > > > Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com> > > --- > > package/network/services/dnsmasq/Makefile | 2 +- > > package/network/services/dnsmasq/files/dnsmasq.init | 7 +------ > > 2 files changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile > > index d51f23e48b..5e83b4f8e5 100644 > > --- a/package/network/services/dnsmasq/Makefile > > +++ b/package/network/services/dnsmasq/Makefile > > @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk > > > > PKG_NAME:=dnsmasq > > PKG_VERSION:=2.80 > > -PKG_RELEASE:=8 > > +PKG_RELEASE:=9 > > > > PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.xz > > PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq > > diff --git a/package/network/services/dnsmasq/files/dnsmasq.init b/package/network/services/dnsmasq/files/dnsmasq.init > > index 05c555bd8c..22471c11aa 100644 > > --- a/package/network/services/dnsmasq/files/dnsmasq.init > > +++ b/package/network/services/dnsmasq/files/dnsmasq.init > > @@ -888,7 +888,6 @@ dnsmasq_start() > > config_get resolvfile "$cfg" resolvfile > > [ -n "$resolvfile" -a ! -e "$resolvfile" ] && touch "$resolvfile" > > xappend "--resolv-file=$resolvfile" > > - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && localuse=1 > By removing this line 127.0.0.1 is not added anymore in > /tmp/resolv.conf as localuse is 0 by default; > as a result local resolving is broken. Yes, this is exactly the behavior change. It intends to make it clear that to use dnsmasq for local dns query, one has to set explicitly localuse to 1. And with localuse being 0, /etc/resolv.conf will default to /tmp/resolv.conf.auto as setup by /etc/init.d/boot The argument is mainly that resolvfile being /tmp/resolv.conf.auto should not have a consequence on the content of /tmp/resolv.conf. resolvfile is dnsmasq specific and /tmp/resolv.conf is for all other processes in the local system Should have made it more clear in the commit message that this can break existing expectations. Sorry about that, I will drop this change, amend the first, then send another version. Thank you, yousong
On Mon, Feb 18, 2019 at 7:52 AM Yousong Zhou <yszhou4tech@gmail.com> wrote: > > On Mon, 18 Feb 2019 at 04:00, Hans Dedecker <dedeckeh@gmail.com> wrote: > > > > Hi Yousong, > > On Thu, Feb 14, 2019 at 1:15 PM Yousong Zhou <yszhou4tech@gmail.com> wrote:Y > > > > > > Option resolvfile should now affect only how dnsmasq itself will run. > > > The implicit effect of its specific value on other parts of the system > > > makes the code unnecessarily hard to follow > > > > > > Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com> > > > --- > > > package/network/services/dnsmasq/Makefile | 2 +- > > > package/network/services/dnsmasq/files/dnsmasq.init | 7 +------ > > > 2 files changed, 2 insertions(+), 7 deletions(-) > > > > > > diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile > > > index d51f23e48b..5e83b4f8e5 100644 > > > --- a/package/network/services/dnsmasq/Makefile > > > +++ b/package/network/services/dnsmasq/Makefile > > > @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk > > > > > > PKG_NAME:=dnsmasq > > > PKG_VERSION:=2.80 > > > -PKG_RELEASE:=8 > > > +PKG_RELEASE:=9 > > > > > > PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.xz > > > PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq > > > diff --git a/package/network/services/dnsmasq/files/dnsmasq.init b/package/network/services/dnsmasq/files/dnsmasq.init > > > index 05c555bd8c..22471c11aa 100644 > > > --- a/package/network/services/dnsmasq/files/dnsmasq.init > > > +++ b/package/network/services/dnsmasq/files/dnsmasq.init > > > @@ -888,7 +888,6 @@ dnsmasq_start() > > > config_get resolvfile "$cfg" resolvfile > > > [ -n "$resolvfile" -a ! -e "$resolvfile" ] && touch "$resolvfile" > > > xappend "--resolv-file=$resolvfile" > > > - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && localuse=1 > > By removing this line 127.0.0.1 is not added anymore in > > /tmp/resolv.conf as localuse is 0 by default; > > as a result local resolving is broken. > > Yes, this is exactly the behavior change. It intends to make it clear > that to use dnsmasq for local dns query, one has to set explicitly > localuse to 1. And with localuse being 0, /etc/resolv.conf will > default to /tmp/resolv.conf.auto as setup by /etc/init.d/boot This will mean a change in behavior when an user upgrades as localuse will default to 0 which will mean for local dns queries dnsmasq will be bypassed as the local dns requests will be forwarded to the dns servers in /tmp/resolv.conf.auto I understand the motivation for the change but shouldn't we try to preserve the behavior when an user upgrades ? Hans > > The argument is mainly that resolvfile being /tmp/resolv.conf.auto > should not have a consequence on the content of /tmp/resolv.conf. > resolvfile is dnsmasq specific and /tmp/resolv.conf is for all other > processes in the local system > > Should have made it more clear in the commit message that this can > break existing expectations. Sorry about that, I will drop this > change, amend the first, then send another version. > > Thank you, > yousong
diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile index d51f23e48b..5e83b4f8e5 100644 --- a/package/network/services/dnsmasq/Makefile +++ b/package/network/services/dnsmasq/Makefile @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk PKG_NAME:=dnsmasq PKG_VERSION:=2.80 -PKG_RELEASE:=8 +PKG_RELEASE:=9 PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.xz PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq diff --git a/package/network/services/dnsmasq/files/dnsmasq.init b/package/network/services/dnsmasq/files/dnsmasq.init index 05c555bd8c..22471c11aa 100644 --- a/package/network/services/dnsmasq/files/dnsmasq.init +++ b/package/network/services/dnsmasq/files/dnsmasq.init @@ -888,7 +888,6 @@ dnsmasq_start() config_get resolvfile "$cfg" resolvfile [ -n "$resolvfile" -a ! -e "$resolvfile" ] && touch "$resolvfile" xappend "--resolv-file=$resolvfile" - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && localuse=1 fi config_get hostsfile "$cfg" dhcphostsfile @@ -1039,13 +1038,9 @@ dnsmasq_start() dnsmasq_stop() { local cfg="$1" - local noresolv resolvfile localuse + local localuse - config_get_bool noresolv "$cfg" noresolv 0 config_get_bool localuse "$cfg" localuse 0 - config_get resolvfile "$cfg" "resolvfile" - - [ "$noresolv" = 0 -a "$resolvfile" = "/tmp/resolv.conf.auto" ] && localuse=1 [ "$localuse" -gt 0 ] && ln -sf "/tmp/resolv.conf.auto" /tmp/resolv.conf rm -f ${BASEDHCPSTAMPFILE}.${cfg}.*.dhcp
Option resolvfile should now affect only how dnsmasq itself will run. The implicit effect of its specific value on other parts of the system makes the code unnecessarily hard to follow Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com> --- package/network/services/dnsmasq/Makefile | 2 +- package/network/services/dnsmasq/files/dnsmasq.init | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-)