[libvirt] [PATCH 0/8] Add --in-place and --check to test-wrap-argv
The --in-place parameter makes the invocation in testutils slightly nicer and --check makes syntax-check faster. Ján Tomko (8): test-wrap-argv: split out rewrap_line test-wrap-argv: split out rewrap_arg test-wrap-argv: return a string in rewrap_arg test-wrap-argv: use map and join instead of a for cycle test-wrap-argv: return a string in rewrap_line test-wrap-argv: hold a copy of the original file in an array test-wrap-argv: add --in-place parameter test-wrap-argv: add --check parameter cfg.mk | 12 +--- tests/test-wrap-argv.pl | 170 ++++++++++++++++++++++++++++++------------------ tests/testutils.c | 8 +-- 3 files changed, 108 insertions(+), 82 deletions(-) -- 2.7.3
Shorten the rewrap subroutine by splitting out the code dealing with a single line. Also remove $file from the warning. --- tests/test-wrap-argv.pl | 116 +++++++++++++++++++++++++----------------------- 1 file changed, 61 insertions(+), 55 deletions(-) diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index a81cf43..3847d7b 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -59,70 +59,76 @@ sub rewrap { # Now each @lines represents a single command, we # can process them foreach my $line (@lines) { - my @bits = split / /, join('', $line); + &rewrap_line ($line); + } + +} - # @bits contains env vars, then the command line - # and then the arguments - my @env; - my $cmd; - my @args; +sub rewrap_line { + my $line = shift; + my @bits = split / /, join('', $line); - if ($bits[0] !~ /=/) { - $cmd = shift @bits; - } + # @bits contains env vars, then the command line + # and then the arguments + my @env; + my $cmd; + my @args; - foreach my $bit (@bits) { - # If no command is defined yet, we must still - # have env vars - if (!defined $cmd) { - # Look for leading / to indicate command name - if ($bit =~ m,^/,) { - $cmd = $bit; - } else { - push @env, $bit; - } + if ($bits[0] !~ /=/) { + $cmd = shift @bits; + } + + foreach my $bit (@bits) { + # If no command is defined yet, we must still + # have env vars + if (!defined $cmd) { + # Look for leading / to indicate command name + if ($bit =~ m,^/,) { + $cmd = $bit; + } else { + push @env, $bit; + } + } else { + # If there's a leading '-' then this is a new + # parameter, otherwise its a value for the prev + # parameter. + if ($bit =~ m,^-,) { + push @args, $bit; } else { - # If there's a leading '-' then this is a new - # parameter, otherwise its a value for the prev - # parameter. - if ($bit =~ m,^-,) { - push @args, $bit; - } else { - $args[$#args] .= " " . $bit; - } + $args[$#args] .= " " . $bit; } } + } - # Print env + command first - print join(" \\\n", @env, $cmd), " \\\n"; - # We might have to split line argument values... - for (my $i = 0; $i <= $#args; $i++) { - my $arg = $args[$i]; - while (length($arg) > 80) { - my $split = rindex $arg, ",", 80; - if ($split == -1) { - $split = rindex $arg, ":", 80; - } - if ($split == -1) { - $split = rindex $arg, " ", 80; - } - if ($split == -1) { - warn "$file: cannot find nice place to split '$arg' below 80 chars\n"; - $split = 79; - } - $split++; - - my $head = substr $arg, 0, $split; - $arg = substr $arg, $split; - - print $head, "\\\n"; + # Print env + command first + print join(" \\\n", @env, $cmd), " \\\n"; + # We might have to split line argument values... + for (my $i = 0; $i <= $#args; $i++) { + my $arg = $args[$i]; + while (length($arg) > 80) { + my $split = rindex $arg, ",", 80; + if ($split == -1) { + $split = rindex $arg, ":", 80; } - print $arg; - if ($i != $#args) { - print " \\\n"; - } else { - print "\n"; + if ($split == -1) { + $split = rindex $arg, " ", 80; + } + if ($split == -1) { + warn "cannot find nice place to split '$arg' below 80 chars\n"; + $split = 79; } + $split++; + + my $head = substr $arg, 0, $split; + $arg = substr $arg, $split; + + print $head, "\\\n"; + } + print $arg; + if ($i != $#args) { + print " \\\n"; + } else { + print "\n"; } } } -- 2.7.3
Split out the code wrapping the single argument. --- tests/test-wrap-argv.pl | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index 3847d7b..72a3e32 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -104,27 +104,8 @@ sub rewrap_line { print join(" \\\n", @env, $cmd), " \\\n"; # We might have to split line argument values... for (my $i = 0; $i <= $#args; $i++) { - my $arg = $args[$i]; - while (length($arg) > 80) { - my $split = rindex $arg, ",", 80; - if ($split == -1) { - $split = rindex $arg, ":", 80; - } - if ($split == -1) { - $split = rindex $arg, " ", 80; - } - if ($split == -1) { - warn "cannot find nice place to split '$arg' below 80 chars\n"; - $split = 79; - } - $split++; - - my $head = substr $arg, 0, $split; - $arg = substr $arg, $split; + &rewrap_arg($args[$i]); - print $head, "\\\n"; - } - print $arg; if ($i != $#args) { print " \\\n"; } else { @@ -132,3 +113,28 @@ sub rewrap_line { } } } + +sub rewrap_arg { + my $arg = shift; + + while (length($arg) > 80) { + my $split = rindex $arg, ",", 80; + if ($split == -1) { + $split = rindex $arg, ":", 80; + } + if ($split == -1) { + $split = rindex $arg, " ", 80; + } + if ($split == -1) { + warn "cannot find nice place to split '$arg' below 80 chars\n"; + $split = 79; + } + $split++; + + my $head = substr $arg, 0, $split; + $arg = substr $arg, $split; + + print $head, "\\\n"; + } + print $arg; +} -- 2.7.3
Do not print anything, let the caller take care of it. --- tests/test-wrap-argv.pl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index 72a3e32..693bed5 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -104,7 +104,7 @@ sub rewrap_line { print join(" \\\n", @env, $cmd), " \\\n"; # We might have to split line argument values... for (my $i = 0; $i <= $#args; $i++) { - &rewrap_arg($args[$i]); + print &rewrap_arg($args[$i]); if ($i != $#args) { print " \\\n"; @@ -116,6 +116,7 @@ sub rewrap_line { sub rewrap_arg { my $arg = shift; + my @ret; while (length($arg) > 80) { my $split = rindex $arg, ",", 80; @@ -131,10 +132,9 @@ sub rewrap_arg { } $split++; - my $head = substr $arg, 0, $split; + push @ret, substr $arg, 0, $split; $arg = substr $arg, $split; - - print $head, "\\\n"; } - print $arg; + push @ret, $arg; + return join("\\\n", @ret); } -- 2.7.3
We have a list of parameters in @args, that need to be rewrapped and separated by a space and escaped newline: " \\\n", with the exception of the last one, which only needs a newline. Instead of a for cycle, rewrap the individual arguments using map, and interleave them with escaped newlines by using join. --- tests/test-wrap-argv.pl | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index 693bed5..4e942cd 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -100,18 +100,10 @@ sub rewrap_line { } } - # Print env + command first - print join(" \\\n", @env, $cmd), " \\\n"; # We might have to split line argument values... - for (my $i = 0; $i <= $#args; $i++) { - print &rewrap_arg($args[$i]); - - if ($i != $#args) { - print " \\\n"; - } else { - print "\n"; - } - } + @args = map { &rewrap_arg($_) } @args; + # Print env + command first + print join(" \\\n", @env, $cmd, @args), "\n"; } sub rewrap_arg { -- 2.7.3
Leave the printing up to &rewrap. --- tests/test-wrap-argv.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index 4e942cd..1f619cc 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -59,7 +59,7 @@ sub rewrap { # Now each @lines represents a single command, we # can process them foreach my $line (@lines) { - &rewrap_line ($line); + print &rewrap_line ($line); } } @@ -103,7 +103,7 @@ sub rewrap_line { # We might have to split line argument values... @args = map { &rewrap_arg($_) } @args; # Print env + command first - print join(" \\\n", @env, $cmd, @args), "\n"; + return join(" \\\n", @env, $cmd, @args), "\n"; } sub rewrap_arg { -- 2.7.3
This will be useful to check if the file is wrapped already. --- tests/test-wrap-argv.pl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index 1f619cc..96f998a 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -33,15 +33,15 @@ sub rewrap { # Read the original file open FILE, "<", $file or die "cannot read $file: $!"; - my @lines; - while (<FILE>) { + my @orig_lines = <FILE>; + close FILE; + my @lines = @orig_lines; + foreach (@lines) { # If there is a trailing '\' then kill the new line if (/\\$/) { chomp; $_ =~ s/\\$//; } - - push @lines, $_; } # Skip empty files @@ -49,7 +49,6 @@ sub rewrap { # Kill the last new line in the file chomp @lines[$#lines]; - close FILE; # Reconstruct the master data by joining all lines # and then split again based on the real desired -- 2.7.3
If --in-place is supplied as the first argument to the script, replace the file in-place instead of printing to stdout. --- tests/test-wrap-argv.pl | 24 +++++++++++++++++++++--- tests/testutils.c | 8 +------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index 96f998a..97f6903 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -22,7 +22,16 @@ # of the file. Parameter values that are longer than 80 chars will # also be split. # +# If --in-place is supplied as the first parameter of this script, +# the files will be changed in place. +# Otherwise the rewrapped files are printed to the standard output. +$in_place = 0; + +if (@ARGV[0] eq "--in-place") { + $in_place = 1; + shift @ARGV; +} foreach my $file (@ARGV) { &rewrap($file); @@ -57,10 +66,19 @@ sub rewrap { # Now each @lines represents a single command, we # can process them - foreach my $line (@lines) { - print &rewrap_line ($line); - } + @lines = map { &rewrap_line($_) } @lines; + if ($in_place) { + open FILE, ">", $file or die "cannot write $file: $!"; + foreach my $line (@lines) { + print FILE $line; + } + close FILE; + } else { + foreach my $line (@lines) { + print $line; + } + } } sub rewrap_line { diff --git a/tests/testutils.c b/tests/testutils.c index 54adab2..1d503c1 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -440,26 +440,20 @@ static int virTestRewrapFile(const char *filename) { int ret = -1; - char *outbuf = NULL; char *script = NULL; virCommandPtr cmd = NULL; if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0) goto cleanup; - cmd = virCommandNewArgList(script, filename, NULL); - virCommandSetOutputBuffer(cmd, &outbuf); + cmd = virCommandNewArgList(script, "--in-place", filename, NULL); if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (virFileWriteStr(filename, outbuf, 0666) < 0) - goto cleanup; - ret = 0; cleanup: VIR_FREE(script); virCommandFree(cmd); - VIR_FREE(outbuf); return ret; } -- 2.7.3
This script can already operate on a list of files. Add a --check parameter to check if multiple files are wrapped correctly with a single invocation of the script. --- cfg.mk | 12 +----------- tests/test-wrap-argv.pl | 23 ++++++++++++++++++++++- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/cfg.mk b/cfg.mk index 297ca3a..f48c035 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1108,17 +1108,7 @@ spacing-check: test-wrap-argv: $(AM_V_GEN)files=`$(VC_LIST) | grep -E '\.(ldargs|args)'`; \ - for file in $$files ; \ - do \ - $(PERL) $(top_srcdir)/tests/test-wrap-argv.pl $$file > $${file}-t ; \ - diff $$file $${file}-t; \ - res=$$? ; \ - rm $${file}-t ; \ - test $$res == 0 || { \ - echo "$(ME): Incorrect line wrapping in $$file" 1>&2; \ - echo "$(ME): Use test-wrap-argv.pl to wrap test data files" 1>&2; \ - exit 1; } \ - done + $(PERL) $(top_srcdir)/tests/test-wrap-argv.pl --check $$files # sc_po_check can fail if generated files are not built first sc_po_check: \ diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl index 97f6903..d66f5b4 100755 --- a/tests/test-wrap-argv.pl +++ b/tests/test-wrap-argv.pl @@ -24,19 +24,30 @@ # # If --in-place is supplied as the first parameter of this script, # the files will be changed in place. +# If --check is the first parameter, the script will return +# a non-zero value if a file is not wrapped correctly. # Otherwise the rewrapped files are printed to the standard output. $in_place = 0; +$check = 0; if (@ARGV[0] eq "--in-place") { $in_place = 1; shift @ARGV; +} elsif (@ARGV[0] eq "--check") { + $check = 1; + shift @ARGV; } foreach my $file (@ARGV) { - &rewrap($file); + $ret = 0; + if (&rewrap($file) < 0) { + $ret = 1; + } } +exit $ret; + sub rewrap { my $file = shift; @@ -74,11 +85,21 @@ sub rewrap { print FILE $line; } close FILE; + } elsif ($check) { + my $nl = join('', @lines); + my $ol = join('', @orig_lines); + unless ($nl eq $ol) { + print STDERR $ol; + print STDERR "Incorrect line wrapping in $file\n"; + print STDERR "Use test-wrap-argv.pl to wrap test data files\n"; + return -1; + } } else { foreach my $line (@lines) { print $line; } } + return 0; } sub rewrap_line { -- 2.7.3
On 06/15/2016 12:39 PM, Ján Tomko wrote:
The --in-place parameter makes the invocation in testutils slightly nicer and --check makes syntax-check faster.
Ján Tomko (8): test-wrap-argv: split out rewrap_line test-wrap-argv: split out rewrap_arg test-wrap-argv: return a string in rewrap_arg test-wrap-argv: use map and join instead of a for cycle test-wrap-argv: return a string in rewrap_line test-wrap-argv: hold a copy of the original file in an array test-wrap-argv: add --in-place parameter test-wrap-argv: add --check parameter
cfg.mk | 12 +--- tests/test-wrap-argv.pl | 170 ++++++++++++++++++++++++++++++------------------ tests/testutils.c | 8 +-- 3 files changed, 108 insertions(+), 82 deletions(-)
I believe one other difference is that "all" the differences or issues are generated and displayed not just one at a time (which I actually prefer). Changes look OK to me; however, I think an "existing" issue is that the splitting is off by 1 or 2 characters. If a ",", ":", or " " is the 80th character and then a " \" is added, that addition is on the next line if you have a an 80 column display (likewise a "\" when there are more args to be printed). Check out the following files and you'll note by visual inspection there are lines that are longer than 80: tests/qemuargv2xmldata/qemuargv2xml-disk-drive-cache-unsafe.args tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-ipv6.args tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-default-nic.args tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-http.args tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-tftp.args tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ipv6.args tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages5.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args tests/qemuxml2argvdata/qemuxml2argv-name-escape.args tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-addr.args tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-idx.args tests/qemuxml2argvdata/qemuxml2argv-pci-many.args tests/qemuxml2argvdata/qemuxml2argv-pcie-expander-bus.args All I did was change 79 to 78 and 80 to 79 (it gets worse with 78/77) ACK to what's done though... John
On Tue, Jun 21, 2016 at 06:41:35AM -0400, John Ferlan wrote:
On 06/15/2016 12:39 PM, Ján Tomko wrote:
The --in-place parameter makes the invocation in testutils slightly nicer and --check makes syntax-check faster.
Ján Tomko (8): test-wrap-argv: split out rewrap_line test-wrap-argv: split out rewrap_arg test-wrap-argv: return a string in rewrap_arg test-wrap-argv: use map and join instead of a for cycle test-wrap-argv: return a string in rewrap_line test-wrap-argv: hold a copy of the original file in an array test-wrap-argv: add --in-place parameter test-wrap-argv: add --check parameter
cfg.mk | 12 +--- tests/test-wrap-argv.pl | 170 ++++++++++++++++++++++++++++++------------------ tests/testutils.c | 8 +-- 3 files changed, 108 insertions(+), 82 deletions(-)
I believe one other difference is that "all" the differences or issues are generated and displayed not just one at a time (which I actually prefer).
Changes look OK to me; however, I think an "existing" issue is that the splitting is off by 1 or 2 characters. If a ",", ":", or " " is the 80th character and then a " \" is added, that addition is on the next line if you have a an 80 column display (likewise a "\" when there are more args to be printed).
Check out the following files and you'll note by visual inspection there are lines that are longer than 80:
tests/qemuargv2xmldata/qemuargv2xml-disk-drive-cache-unsafe.args tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-ipv6.args tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-default-nic.args tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-http.args tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-tftp.args tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ipv6.args tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages5.args tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args tests/qemuxml2argvdata/qemuxml2argv-name-escape.args tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-addr.args tests/qemuxml2argvdata/qemuxml2argv-pci-autoadd-idx.args tests/qemuxml2argvdata/qemuxml2argv-pci-many.args tests/qemuxml2argvdata/qemuxml2argv-pcie-expander-bus.args
All I did was change 79 to 78 and 80 to 79 (it gets worse with 78/77)
ACK to what's done though...
Thanks, I've pushed the series and marked the issues on my TODO list. Jan
participants (2)
-
John Ferlan -
Ján Tomko