[pve-devel] [PATCH common v2 2/2] Fix #1234: allows multiple search domains to be set

[Emmanuel Kasper]

First thank you for taking the time to review this.

On 04/11/2017 11:47 AM, Fabian Grünbichler wrote:
> meta: has the previous input about other code paths assuming
> $rc->{search} containing a single entry been addressed?

Yes.
ack resolvconf /usr/share/perl5/PVE/*

brings API2/OpenVZ.pm, API2/Nodes.pm, Cluster.pm and LXC/Setup.pm

* ignoring OpenVZ.pm,
* Nodes.pm only passes the read parameter as is to API clients,
* Cluster.pm expects a single entry ( hence the need to be patched
first, this can be done independently)
* LXC/Setup.pm expect can work with multiple search domains
$searchdomains = $host_resolv_conf->{search};


> On Tue, Apr 04, 2017 at 03:26:24PM +0200, Emmanuel Kasper wrote:
> 
> some short description of this commit would be nice here, especially
> regarding the comments/questions below ;)
> 
>> ---
>>  src/PVE/INotify.pm | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
>> index 074f8b3..5f08439 100644
>> --- a/src/PVE/INotify.pm
>> +++ b/src/PVE/INotify.pm
>> @@ -539,11 +539,16 @@ sub read_etc_resolv_conf {
>>      my $nscount = 0;
>>      while (my $line = <$fh>) {
>>  	chomp $line;
>> -	# resolv.conf should *either* have a search or domain, else behaviour is undefined
>> -	# see res_init.c in libc, havesearch is set to 0 when a domain entry is found
>> -	if ($line =~ m/^(search|domain)\s+(\S+)\s*/) {
>> -	    $res->{search} = $2;
>> -	} elsif ($line =~ m/^\s*nameserver\s+($PVE::Tools::IPRE)\s*/) {
>> +	# resolv.conf should *either* have a search or domain, else
>> +	# last one found wins, see resolv.conf(5)
> 
> resolv.conf(5) also mentions the following limitation for the search
> option (both in Jessie and Stretch):
> 
> The search list is currently limited to six domains with a total of 256
> characters.
> 
> maybe adding a check for that would make sense?
> 
>> +	if ($line =~ m/^domain\s+(\S+)\s*$/) {
>> +	    $res->{search} = $1;
> 
> this actually became stricter than the old version - previously the
> (invalid)
> 
> domain one two
> 
> would have been parsed as "domain one", now it is skipped because the RE
> does not match.
> 
> based on the previous feedback I think this is intentional - but it
> would be nice to make this explicit in the commit message
> 
>> +	}
>> +	elsif ($line =~ m/^search\s+((?:\S+\s*)*)$/) {
>> +	    (my $domains = $1) =~ s/\s+$//;
> 
> this would allow an empty search list - does that make sense? it is
> again a change in bevaviour, but this time in the other direction ;)

maybe we should find on a general direction on the question "what to
do with invalid entries in /etc/resolv.conf" ?

my answer would be: just like the libc resolver, silently ignores all
wrong entries, extract the correct ones ?
what do you think ?

that would mean concretly instead of

> if ($line =~ m/^domain\s+(\S+)\s*$/)
capture the first 6 words

and instead of
> if ($line =~ m/^domain\s+(\S+)\s*$/)
we should capture the first word when multiple entries are present

>> +	elsif ($line =~ m/^search\s+((?:\S+\s*)*)$/) {
>> +	    (my $domains = $1) =~ s/\s+$//;
>
> this would allow an empty search list - does that make sense? it is
> again a change in bevaviour, but this time in the other direction ;)

an empty search list does not make sense and should not taken as valid
match (just like the resolver does) again your regexp makes sense