| Message ID | 1418779273-15111-1-git-send-email-yszhou4tech@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
applied, thanks.
Hi yousong, thanks for the patch. A few issues/comments: Am 17.12.2014 um 02:21 schrieb Yousong Zhou: > Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com> > --- > package/network/services/dnsmasq/Makefile | 27 ++++++++++++++++++-- > .../network/services/dnsmasq/files/dnsmasq.init | 5 ++++ > 2 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile > index a530225..2da593d 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.72 > -PKG_RELEASE:=1 > +PKG_RELEASE:=2 > > PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz > PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq > @@ -72,6 +72,26 @@ define Package/dnsmasq/conffiles > /etc/dnsmasq.conf > endef > > +define Package/dnsmasq/config/Default > + if PACKAGE_$(1)-$(2) > + config PACKAGE_dnsmasq_$(2)_dhcpv6 > + bool "Build with DHCPv6 support." > + default y This config needs to depend on IPV6 > + config PACKAGE_dnsmasq_$(2)_dnssec > + bool "Build with DNSSEC support." > + default y This config needs to select PACKAGE_libnettle > + config PACKAGE_dnsmasq_$(2)_auth > + bool "Build with the facility to act as an authoritative DNS server." > + default y > + config PACKAGE_dnsmasq_$(2)_ipset > + bool "Build with ipset support." > + select PACKAGE_kmod-ipt-ipset > + default y > + endif > +endef It would be nice if this would appear in a submenu and only if the dnsmasq package is enabled. > + > +Package/dnsmasq-full/config=$(call Package/dnsmasq/config/Default,dnsmasq,full) > + > Package/dnsmasq-dhcpv6/conffiles = $(Package/dnsmasq/conffiles) > Package/dnsmasq-full/conffiles = $(Package/dnsmasq/conffiles) > > @@ -85,7 +105,10 @@ ifeq ($(BUILD_VARIANT),nodhcpv6) > endif > > ifeq ($(BUILD_VARIANT),full) > - COPTS += -DHAVE_DNSSEC > + COPTS += $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_dhcpv6),,-DNO_DHCP6) \ > + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_dnssec),-DHAVE_DNSSEC) \ > + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_auth),,-DNO_AUTH) \ > + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_ipset),,-DNO_IPSET) > COPTS += $(if $(CONFIG_LIBNETTLE_MINI),-DNO_GMP,) > else > COPTS += -DNO_AUTH -DNO_IPSET The installation section Package/dnsmasq-full/install should be modified, too: if DNSSEC is not enabled, there's no need to install trust-anchors.conf --- The patch leaves the main problem unsolved: you still have to enable the whole IPv6 stuff for DNS Auth / DNSSEC / IPSET support. I personally have no problems with migrating from package variants to the "configure at build time" approach. But I don't think they should be mixed together. The "dnsmasq-full" package with only dhcpv6 enabled is exactly the same as the "dnsmasq-dhcpv6" package variant. And as soon as you deselect one of the config options, "dnsmasq-full" actually isn't a "full" package anymore, right ? ;-) IMHO too much inconsistencies. If we go this way, I would prefer having just a single (fully configurable) dnsmasq package. I'll send a patch later this evening. Regards, Frank > diff --git a/package/network/services/dnsmasq/files/dnsmasq.init b/package/network/services/dnsmasq/files/dnsmasq.init > index 942acd7..209952b 100644 > --- a/package/network/services/dnsmasq/files/dnsmasq.init > +++ b/package/network/services/dnsmasq/files/dnsmasq.init > @@ -85,6 +85,10 @@ append_address() { > xappend "--address=$1" > } > > +append_ipset() { > + xappend "--ipset=$1" > +} > + > append_interface() { > local ifname=$(uci_get_state network "$1" ifname "$1") > xappend "--interface=$ifname" > @@ -135,6 +139,7 @@ dnsmasq() { > append_parm "$cfg" "local" "--server" > config_list_foreach "$cfg" "server" append_server > config_list_foreach "$cfg" "address" append_address > + config_list_foreach "$cfg" "ipset" append_ipset > config_list_foreach "$cfg" "interface" append_interface > config_list_foreach "$cfg" "notinterface" append_notinterface > config_list_foreach "$cfg" "addnhosts" append_addnhosts
Hi, Frank. On 18 December 2014 at 01:49, Frank Schäfer <fschaefer.oss@googlemail.com> wrote: > Hi yousong, > > thanks for the patch. > A few issues/comments: > > Am 17.12.2014 um 02:21 schrieb Yousong Zhou: >> Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com> >> --- >> package/network/services/dnsmasq/Makefile | 27 ++++++++++++++++++-- >> .../network/services/dnsmasq/files/dnsmasq.init | 5 ++++ >> 2 files changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile >> index a530225..2da593d 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.72 >> -PKG_RELEASE:=1 >> +PKG_RELEASE:=2 >> >> PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz >> PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq >> @@ -72,6 +72,26 @@ define Package/dnsmasq/conffiles >> /etc/dnsmasq.conf >> endef >> >> +define Package/dnsmasq/config/Default >> + if PACKAGE_$(1)-$(2) >> + config PACKAGE_dnsmasq_$(2)_dhcpv6 >> + bool "Build with DHCPv6 support." >> + default y > This config needs to depend on IPV6 > >> + config PACKAGE_dnsmasq_$(2)_dnssec >> + bool "Build with DNSSEC support." >> + default y > This config needs to select PACKAGE_libnettle > >> + config PACKAGE_dnsmasq_$(2)_auth >> + bool "Build with the facility to act as an authoritative DNS server." >> + default y >> + config PACKAGE_dnsmasq_$(2)_ipset >> + bool "Build with ipset support." >> + select PACKAGE_kmod-ipt-ipset >> + default y >> + endif >> +endef > It would be nice if this would appear in a submenu and only if the > dnsmasq package is enabled. This currently only appear if dnsmasq-full is selected. > >> + >> +Package/dnsmasq-full/config=$(call Package/dnsmasq/config/Default,dnsmasq,full) >> + >> Package/dnsmasq-dhcpv6/conffiles = $(Package/dnsmasq/conffiles) >> Package/dnsmasq-full/conffiles = $(Package/dnsmasq/conffiles) >> >> @@ -85,7 +105,10 @@ ifeq ($(BUILD_VARIANT),nodhcpv6) >> endif >> >> ifeq ($(BUILD_VARIANT),full) >> - COPTS += -DHAVE_DNSSEC >> + COPTS += $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_dhcpv6),,-DNO_DHCP6) \ >> + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_dnssec),-DHAVE_DNSSEC) \ >> + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_auth),,-DNO_AUTH) \ >> + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_ipset),,-DNO_IPSET) >> COPTS += $(if $(CONFIG_LIBNETTLE_MINI),-DNO_GMP,) >> else >> COPTS += -DNO_AUTH -DNO_IPSET > The installation section Package/dnsmasq-full/install should be > modified, too: > if DNSSEC is not enabled, there's no need to install trust-anchors.conf > > --- > > The patch leaves the main problem unsolved: > you still have to enable the whole IPv6 stuff for DNS Auth / DNSSEC / > IPSET support. > > I personally have no problems with migrating from package variants to > the "configure at build time" approach. > But I don't think they should be mixed together. > The "dnsmasq-full" package with only dhcpv6 enabled is exactly the same > as the "dnsmasq-dhcpv6" package variant. > And as soon as you deselect one of the config options, "dnsmasq-full" > actually isn't a "full" package anymore, right ? ;-) > IMHO too much inconsistencies. > If we go this way, I would prefer having just a single (fully > configurable) dnsmasq package. Another configurable variant should be better. I just noticed that if dnsmasq-full is selected, all of its dependencies like kmod-ipv6 and libnettle are still selected and compiled in even when IPV6 and dnssec are de-selected. > > I'll send a patch later this evening. Thank you for the comments.
diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile index a530225..2da593d 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.72 -PKG_RELEASE:=1 +PKG_RELEASE:=2 PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq @@ -72,6 +72,26 @@ define Package/dnsmasq/conffiles /etc/dnsmasq.conf endef +define Package/dnsmasq/config/Default + if PACKAGE_$(1)-$(2) + config PACKAGE_dnsmasq_$(2)_dhcpv6 + bool "Build with DHCPv6 support." + default y + config PACKAGE_dnsmasq_$(2)_dnssec + bool "Build with DNSSEC support." + default y + config PACKAGE_dnsmasq_$(2)_auth + bool "Build with the facility to act as an authoritative DNS server." + default y + config PACKAGE_dnsmasq_$(2)_ipset + bool "Build with ipset support." + select PACKAGE_kmod-ipt-ipset + default y + endif +endef + +Package/dnsmasq-full/config=$(call Package/dnsmasq/config/Default,dnsmasq,full) + Package/dnsmasq-dhcpv6/conffiles = $(Package/dnsmasq/conffiles) Package/dnsmasq-full/conffiles = $(Package/dnsmasq/conffiles) @@ -85,7 +105,10 @@ ifeq ($(BUILD_VARIANT),nodhcpv6) endif ifeq ($(BUILD_VARIANT),full) - COPTS += -DHAVE_DNSSEC + COPTS += $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_dhcpv6),,-DNO_DHCP6) \ + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_dnssec),-DHAVE_DNSSEC) \ + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_auth),,-DNO_AUTH) \ + $(if $(CONFIG_PACKAGE_dnsmasq_$(BUILD_VARIANT)_ipset),,-DNO_IPSET) COPTS += $(if $(CONFIG_LIBNETTLE_MINI),-DNO_GMP,) else COPTS += -DNO_AUTH -DNO_IPSET diff --git a/package/network/services/dnsmasq/files/dnsmasq.init b/package/network/services/dnsmasq/files/dnsmasq.init index 942acd7..209952b 100644 --- a/package/network/services/dnsmasq/files/dnsmasq.init +++ b/package/network/services/dnsmasq/files/dnsmasq.init @@ -85,6 +85,10 @@ append_address() { xappend "--address=$1" } +append_ipset() { + xappend "--ipset=$1" +} + append_interface() { local ifname=$(uci_get_state network "$1" ifname "$1") xappend "--interface=$ifname" @@ -135,6 +139,7 @@ dnsmasq() { append_parm "$cfg" "local" "--server" config_list_foreach "$cfg" "server" append_server config_list_foreach "$cfg" "address" append_address + config_list_foreach "$cfg" "ipset" append_ipset config_list_foreach "$cfg" "interface" append_interface config_list_foreach "$cfg" "notinterface" append_notinterface config_list_foreach "$cfg" "addnhosts" append_addnhosts
Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com> --- package/network/services/dnsmasq/Makefile | 27 ++++++++++++++++++-- .../network/services/dnsmasq/files/dnsmasq.init | 5 ++++ 2 files changed, 30 insertions(+), 2 deletions(-)