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

[Fabian Grünbichler]

meta: has the previous input about other code paths assuming
$rc->{search} containing a single entry been addressed?

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 ;)

if not, the RE could become:
/^search\s+(\S+(\s+\S+)*)\s*$/

with no further substitution needed?

> +	    $res->{search} = $domains;
> +	}
> +	elsif ($line =~ m/^\s*nameserver\s+($PVE::Tools::IPRE)\s*/) {
>  	    $nscount++;
>  	    if ($nscount <= 3) {
>  		$res->{"dns$nscount"} = $1;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel