[LEDE-DEV] [PATCH] dnsmasq: write resolv.conf also when noresolv = 1

[Hans Dedecker]

On Thu, Jun 1, 2017 at 12:00 PM, Paul Oranje <por at 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 <dedeckeh at gmail.com> het volgende geschreven:
>>
>> On Sat, May 20, 2017 at 12:41 AM, Paul Oranje <por at 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 <por at 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 at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/lede-dev
>>
>> _______________________________________________
>> Lede-dev mailing list
>> Lede-dev at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
>