On Thu, Jun 1, 2017 at 12:00 PM, Paul Oranje <p...@xs4all.nl> wrote:
> Hello Hans,
>
> A new version of this small patch is worked on -overlooked your previous
> comment and have lately been busy with other stuff-, but after studying the
> code a little more I’ve a few questions. The intended patch changes code that
> was added with commit a35f9bbc43c3da06eed042f80dc09e8c1da681b4 (dnsmasq:
> Multiple dnsmasq instances support) that was authored by you.
>
>
> The routines dnsmasq_start() and dnsmasq_stop() contain a guard on writing
> and resetting /tmp/resolv.conf:
> [ "$resolvfile" = "/tmp/resolv.conf.auto” ] &&
>
> As explained in FS#785, the guard test fails when $noresolv is true and
> $resolvfile has not been explicitly configured to its default
> "/tmp/resolv.conf.auto", causing /tmp/resolv.conf to remain a soft link to
> /tmp/resolv.conf.auto (the WAN its DNS).
>
> The routine dnsmasq_stop() the guard is commented with
> #relink /tmp/resolve.conf only for main instance
>
> This suggests that only for the main (first ?) instance /tmp/resolv.conf
> would be handled, which makes sense, but the test to accomplish that seems
> wrong.
>
> Q1:
> What is the supposed relation between dnsmasq instance and the value of the
> value/setting of resolvfile ?
The DNS servers learned on the wan interface(s) by netifd are written
in /tmp/resolv.conf.auto. By default a dnsmasq instance uses
/tmp/resolv.conf.auto as resolver file unless configured otherwise.
Using /tmp/rsolv.conf.auto as resolver file triggers logic to rewrite
/tmp/resolv.conf
>
>
> A fix I considered is to omit this guard altogether (so different from the
> patch previously submitted), so that the local DNS service will allways be
> used for local (router) DNS resolution when dnsmasq is started; at stop of
> dnsmasq the local resolution would return to use $resolvfile. Obviously doing
> so in dnsmasq_start() and dnsmasq_stop() routines that start or stop an
> instance would be the wrong place to do so, since that code will be called
> for each instance. These routines ar spawned from the start_service() and
> stop_service() routines that do iterate on the instances.
>
> Q2:
> Would placement of the /tmp/resolv.conf handling not be better placed in the
> start_ and stop_service() routines or, be guarded by a better test in the
> dnsmasq_start() and _stop() routines ? In other words: what would be a
> correct test ?
we could move rewriting of /tmp/resolv.conf to the start routine; but
this will not be trivial as for every dnsmasq instance DNS_SERVERS and
DOMAIN state will need to be kept
>
>
> In the stop routine the file /tmp/resolv.conf is (re)set to a soft link to
> $resolvfile, which currently because of the test always is
> "/tmp/resolv.conf.auto”, irrespective whether that file exists or not.
>
> Q3:
> What would be the desired state of /tmp/resolv.conf after dnsmasq has been
> stopped ?
In case dnsmasq is stopped /tmp/resolv.conf needs to use only the wan
DNS servers; which are written by netifd in /tmp/resolv.conf.auto; and
not anymore 127.0.0.1.
Hans
>
>
> Bye,
> Paul
>
>
>
>> Op 20 mei 2017, om 20:58 heeft Hans Dedecker <dedec...@gmail.com> het
>> volgende geschreven:
>>
>> On Sat, May 20, 2017 at 12:41 AM, Paul Oranje <p...@xs4all.nl> wrote:
>>> When UCI dhcp.dnsmasq.noresolv is true, dnsmasq ignores the (wan)
>>> resolv.conf
>>> for upstream name resolution and the dnsmasq init script ialso skips writing
>>> /tmp/resolv.conf (/etc/resolv.conf soft links that file).
>>>
>>> Not using the local running DNS service when noresolv is true does not make
>>> sence; the semantics of that config value do not imply this. With this patch
>>> the init script also writes /tmp/resolv.conf when noresolv is true.
>>>
>>> fixes FS#785
>>>
>>> Signed-off-by: Paul Oranje <p...@xs4all.nl>
>>> ---
>>> This patch replaces an earlier patch with subject
>>> dnsmasq: also write /tmp/resolv.conf when UCI dhcp.dnsmasq.noresolv is '1'
>>> which is obsoleted because that subject was required to be shorter.
>>> ---
>>> package/network/services/dnsmasq/files/dnsmasq.init | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/package/network/services/dnsmasq/files/dnsmasq.init
>>> b/package/network/services/dnsmasq/files/dnsmasq.init
>>> index 30fec7a4ee..197aae9de1 100644
>>> --- a/package/network/services/dnsmasq/files/dnsmasq.init
>>> +++ b/package/network/services/dnsmasq/files/dnsmasq.init
>>> @@ -947,7 +947,7 @@ dnsmasq_start()
>>> echo >> $CONFIGFILE_TMP
>>> mv -f $CONFIGFILE_TMP $CONFIGFILE
>>>
>>> - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && {
>>> + [ "$noresolv" = "1" -o "$resolvfile" = "/tmp/resolv.conf.auto" ] &&
>>> {
>>> rm -f /tmp/resolv.conf
>>> [ $ADD_LOCAL_DOMAIN -eq 1 ] && [ -n "$DOMAIN" ] && {
>>> echo "search $DOMAIN" >> /tmp/resolv.conf
>>> @@ -982,7 +982,7 @@ dnsmasq_stop()
>>> config_get resolvfile "$cfg" "resolvfile"
>>>
>>> #relink /tmp/resolve.conf only for main instance
>>> - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && {
>>> + [ "$noresolv" = "1" -o "$resolvfile" = "/tmp/resolv.conf.auto" ] &&
>>> {
>> As mentioned in the previous code review noresolv value must be read
>> from config (similar to resolvfile) otherwise this will not work
>>> [ -f /tmp/resolv.conf ] && {
>>> rm -f /tmp/resolv.conf
>>> ln -s "$resolvfile" /tmp/resolv.conf
>> As mentioned in the previous code review resolvfile can now be empty
>> which will make ln -s produce an error
>>
>> Hans
>>> --
>>>
>>>
>>> _______________________________________________
>>> Lede-dev mailing list
>>> Lede-dev@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/lede-dev
>>
>> _______________________________________________
>> Lede-dev mailing list
>> Lede-dev@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
>
_______________________________________________
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
