[pve-devel] [PATCH common v3 1/2] Document current handling of reading and updating resolv.conf
Wolfgang Bumiller
w.bumiller at proxmox.com
Fri Jun 9 13:32:52 CEST 2017
Tests should also be executed from test/Makefile
more comments inline
On Thu, Apr 13, 2017 at 11:21:49AM +0200, Emmanuel Kasper wrote:
> also add unit tests for the two subroutines
> ---
> src/PVE/INotify.pm | 3 +
> test/inotify/resolv.conf_empty_search | 5 ++
> test/inotify/resolv.conf_long_search | 4 +
> test/inotify/resolv.conf_multiple_domain | 5 ++
> test/inotify/resolv.conf_multiple_search | 5 ++
> test/inotify/resolv.conf_search_with_domain_first | 6 ++
> test/inotify/resolv.conf_search_with_domain_last | 6 ++
> test/inotify/resolv.conf_single_domain | 5 ++
> test/inotify/resolv.conf_single_search | 5 ++
> test/inotify/run_inotify_tests.pl | 97 +++++++++++++++++++++++
> 10 files changed, 141 insertions(+)
> create mode 100644 test/inotify/resolv.conf_empty_search
> create mode 100644 test/inotify/resolv.conf_long_search
> create mode 100644 test/inotify/resolv.conf_multiple_domain
> create mode 100644 test/inotify/resolv.conf_multiple_search
> create mode 100644 test/inotify/resolv.conf_search_with_domain_first
> create mode 100644 test/inotify/resolv.conf_search_with_domain_last
> create mode 100644 test/inotify/resolv.conf_single_domain
> create mode 100644 test/inotify/resolv.conf_single_search
> create mode 100755 test/inotify/run_inotify_tests.pl
>
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index b2a5802..074f8b3 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -539,6 +539,8 @@ 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*/) {
> @@ -557,6 +559,7 @@ sub update_etc_resolv_conf {
>
> my $data = "";
>
> + # when writing search domains always write to search, as this allows multiple entries
> $data = "search $resolv->{search}\n"
> if $resolv->{search};
>
> diff --git a/test/inotify/resolv.conf_empty_search b/test/inotify/resolv.conf_empty_search
> new file mode 100644
> index 0000000..342f5ea
> --- /dev/null
> +++ b/test/inotify/resolv.conf_empty_search
> @@ -0,0 +1,5 @@
> +# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)
> +# DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN
I'm not sure we really need dozens of tiny files to have this comment.
In fact, I'm not sure we should use dozens of tiny files at all if their
expected result is in a separate spaghetti code file ;-).
See below for an alternative.
> +nameserver 8.8.4.4
> +nameserver 8.8.8.8
> +search
> diff --git a/test/inotify/resolv.conf_long_search b/test/inotify/resolv.conf_long_search
> new file mode 100644
> index 0000000..b51930b
> --- /dev/null
> +++ b/test/inotify/resolv.conf_long_search
> @@ -0,0 +1,4 @@
> +nameserver 192.168.2.121
> +nameserver 192.168.2.122
> +domain libera.cc proxmox.com
> +search libera.cc.proxmox.com.libera.cc. proxmox.com.libera.cc.proxmox.com.libera.cc.proxmox.com.libera.cci.proxmox.com.libera.cc. proxmox.com.libera.cc.proxmox.com.libera.cc.proxmox.com.libera.cci.proxmox.com.libera.cc.proxmox.com.libera.cc.proxmox libera.cc
> diff --git a/test/inotify/resolv.conf_multiple_domain b/test/inotify/resolv.conf_multiple_domain
> new file mode 100644
> index 0000000..f6e8c01
> --- /dev/null
> +++ b/test/inotify/resolv.conf_multiple_domain
> @@ -0,0 +1,5 @@
> +# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)
> +# DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN
> +nameserver 8.8.4.4
> +nameserver 8.8.8.8
> +domain proxmox.com proxmox.at
> diff --git a/test/inotify/resolv.conf_multiple_search b/test/inotify/resolv.conf_multiple_search
> new file mode 100644
> index 0000000..bda26e9
> --- /dev/null
> +++ b/test/inotify/resolv.conf_multiple_search
> @@ -0,0 +1,5 @@
> +# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)
> +# DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN
> +nameserver 8.8.4.4
> +nameserver 8.8.8.8
> +search proxmox.com proxmox.org proxmox.at
> diff --git a/test/inotify/resolv.conf_search_with_domain_first b/test/inotify/resolv.conf_search_with_domain_first
> new file mode 100644
> index 0000000..de8ec9a
> --- /dev/null
> +++ b/test/inotify/resolv.conf_search_with_domain_first
> @@ -0,0 +1,6 @@
> +# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)
> +# DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN
> +nameserver 8.8.4.4
> +nameserver 8.8.8.8
> +domain libera.cc
> +search proxmox.com
> diff --git a/test/inotify/resolv.conf_search_with_domain_last b/test/inotify/resolv.conf_search_with_domain_last
> new file mode 100644
> index 0000000..3c65392
> --- /dev/null
> +++ b/test/inotify/resolv.conf_search_with_domain_last
> @@ -0,0 +1,6 @@
> +# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)
> +# DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN
> +nameserver 8.8.4.4
> +nameserver 8.8.8.8
> +search proxmox.com proxmox.org
> +domain libera.cc
> diff --git a/test/inotify/resolv.conf_single_domain b/test/inotify/resolv.conf_single_domain
> new file mode 100644
> index 0000000..aaa2b64
> --- /dev/null
> +++ b/test/inotify/resolv.conf_single_domain
> @@ -0,0 +1,5 @@
> +# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)
> +# DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN
> +nameserver 8.8.4.4
> +nameserver 8.8.8.8
> +domain proxmox.com
> diff --git a/test/inotify/resolv.conf_single_search b/test/inotify/resolv.conf_single_search
> new file mode 100644
> index 0000000..1900906
> --- /dev/null
> +++ b/test/inotify/resolv.conf_single_search
> @@ -0,0 +1,5 @@
> +# Dynamic resolv.conf(5) file for glibc resolver(3) generated by resolvconf(8)
> +# DO NOT EDIT THIS FILE BY HAND -- YOUR CHANGES WILL BE OVERWRITTEN
> +nameserver 8.8.4.4
> +nameserver 8.8.8.8
> +search proxmox.com
> diff --git a/test/inotify/run_inotify_tests.pl b/test/inotify/run_inotify_tests.pl
> new file mode 100755
> index 0000000..4dbfb54
> --- /dev/null
> +++ b/test/inotify/run_inotify_tests.pl
> @@ -0,0 +1,97 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
Should add: use lib '../../src'
Or, if you go away from having lots of tiny files, move it out of the
inotify/ subdir and: use lib '../src';
FindBin's $Bin can be dropped then as well (and should be dropped
anyway, just use relative paths).
> +
> +use FindBin '$Bin'; # $Bin is path to current script. Yes, name is strange.
> +use Test::More tests => 15;
> +use IO::Handle;
> +use Data::Dumper;
> +
> +use PVE::INotify;
> +use PVE::Tools;
> +
> +# tests for PVE::INotify::read_etc_resolv_conf and PVE::INotify::update_etc_resolv_conf
> +
> +# reading a resolv.conf with a single entry
> +my $single_domain = join('/', $Bin, 'resolv.conf_single_domain');
> +my $fh = IO::File->new("< $single_domain");
General note: don't use single-parameter style for open functions, pass
'<' and the path separately.
I'd recommend the files to be inline here though:
| open(my $fh, '<', \<<'EOF');
| nameserver 8.8.4.4
| nameserver 8.8.8.8
| domain proxmox.com
| EOF
Now $fh is a file handle reading from an in-memory string which you can
pass to PVE::INotify::read_etc_resolv_conf().
> +my $data = PVE::INotify::read_etc_resolv_conf($single_domain, $fh);
> +
> +is($data->{search}, 'proxmox.com', 'single domain parameter was correctly read');
Alternatively if you do prefer separate files, I'd prefer to also have
the expected results in files easily paired with their source.
Either separate perl scripts, or json files with arrays containing a
member and an expected value (and an optional message) to iterate
through and check. (Or in the update_etc_resolv_conf() part a raw text
file with the new data.)
> +is($data->{dns1}, '8.8.4.4', 'ns1 parameter was correctly read');
> +is($data->{dns2}, '8.8.8.8', 'ns2 parameter was correctly read');
> +
> +$fh->seek(0,0);
> +my @args = ();
> +
> +my $data_to_write = PVE::INotify::update_etc_resolv_conf($single_domain, $fh, $data, @args);
> +
> +is(($data_to_write =~ m/^search proxmox.com\n/), 1, 'single entry was correctly added as search');
> +is(($data_to_write =~ m/\nnameserver 8.8.8.8\n/), 1, 'ns1 parameter was correctly added');
> +is(($data_to_write =~ m/\nnameserver 8.8.4.4\n/), 1, 'ns2 parameter was correctly added');
> +
> +$fh->close();
> +
> +# reading a resolv.conf with a single search entry
> +# we don't retest for nameservers as the test data is the same for those
> +my $single_search = join('/', $Bin, 'resolv.conf_single_search');
> +$fh = IO::File->new("< $single_search");
> +$data = PVE::INotify::read_etc_resolv_conf($single_search, $fh);
> +
> +is($data->{search}, 'proxmox.com', 'single entry search was correctly read');
> +
> +$fh->seek(0,0);
> +$data_to_write = PVE::INotify::update_etc_resolv_conf($single_search, $fh, $data, @args);
> +
> +is(($data_to_write =~ m/^search proxmox.com\n/), 1, 'single entry search was correctly added');
> +
> +$fh->close();
> +
> +# reading a resolv.conf with a search entry containing multiple domains
> +# we don't retest for nameservers as the test data is the same for those
> +my $multiple_search = join('/', $Bin, 'resolv.conf_multiple_search');
> +$fh = IO::File->new("< $multiple_search");
> +$data = PVE::INotify::read_etc_resolv_conf($multiple_search, $fh);
> +
> +is($data->{search}, 'proxmox.com proxmox.org proxmox.at', 'multiple search domains were correctly read');
> +
> +$fh->seek(0,0);
> +$data_to_write = PVE::INotify::update_etc_resolv_conf($multiple_search, $fh, $data, @args);
> +
> +is(($data_to_write =~ m/^search proxmox.com proxmox.org proxmox.at\n/), 1, 'multiple search domains were correctly added');
> +
> +#glic resolver res_init.c will only consider only the last entry
> +#make sure we do the same
> +my $domain_first = join('/', $Bin, 'resolv.conf_search_with_domain_first');
> +$fh = IO::File->new("< $domain_first");
> +$data = PVE::INotify::read_etc_resolv_conf($domain_first, $fh);
> +is($data->{search}, 'proxmox.com', 'read entries when domain is first correct');
> +$fh->seek(0,0);
> +
> +my $domain_last = join('/', $Bin, 'resolv.conf_search_with_domain_last');
> +$fh = IO::File->new("< $domain_last");
> +$data = PVE::INotify::read_etc_resolv_conf($domain_last, $fh);
> +is($data->{search}, 'libera.cc', 'search entries when domain is last');
> +$fh->seek(0,0);
> +
> +my $long_search = join('/', $Bin, 'resolv.conf_long_search');
> +$fh = IO::File->new("< $long_search");
> +$data = PVE::INotify::read_etc_resolv_conf($long_search, $fh);
> +is(defined($data->{search}), '', 'no search entries when search line is more than 255 chars');
> +$fh->seek(0,0);
> +
> +my $empty_search = join('/', $Bin, 'resolv.conf_empty_search');
> +$fh = IO::File->new("< $empty_search");
> +$data = PVE::INotify::read_etc_resolv_conf($empty_search, $fh);
> +is(defined($data->{search}), '', 'no search entries when search line is empty');
> +$fh->seek(0,0);
> +
> +my $multiple_domain = join('/', $Bin, 'resolv.conf_multiple_domain');
> +$fh = IO::File->new("< $multiple_domain");
> +$data = PVE::INotify::read_etc_resolv_conf($multiple_domain, $fh);
> +is($data->{search}, 'proxmox.com', 'first entry of multiple domains in domain correct');
> +$fh->seek(0,0);
> +
> +$fh->close();
> +done_testing();
> --
> 2.1.4
More information about the pve-devel
mailing list