Re: [OSSTEST PATCH v3 1/3] ts-cpupools: new test script

[Ian Campbell <ian.campbell@citrix.com>]

On Sat, 2015-10-03 at 02:39 +0200, Dario Faggioli wrote:
> Copyright (C) 2009-2014 Citrix Inc.

Year.

> +our $default_pool= "Pool-0";
> +our @schedulers= ("credit","credit2","rtds");

I think @schedulers probably ought to come from a runvar (comma-separated).
Consider testing cpupools on 4.4 (which didn't have rtds) or Xen 7.2 which
has the xyzzy scheduler for example.

I'm less sure about $default_pool, I think that one is probably pretty
inherent and not worth generlising?

> +our @cpulist;
> +
> +# Figure out the number of pCPUs of the host. We need to know that for
> +# deciding with what pCPUs we'll create the test pool.
> +sub check_cpus () {
> +  my $xlinfo= target_cmd_output_root($ho, "xl info");
> +  $xlinfo =~ /nr_cpus\s*:\s([0-9]*)/;
> +  $nr_cpus= $1;
> +  logm("Found $nr_cpus pCPUs");
> +  logm("$nr_cpus is yoo few pCPUs for testing cpupools");

"too" and apparently no actual condition check?

But based on discussion on 0/3 I'm hoping this check will go away, or maybe
it will become a die.

> +}
> +
> +# At the beginning:
> +#  * only 1 pool must exist,
> +#  * it must be the default pool.
> +sub check () {
> +  my $cppinfo= target_cmd_output_root($ho, "xl cpupool-list");
> +  my $nr_cpupools= $cppinfo =~ tr/\n//;

The output of "xl cpupool-list" is 
----
Name               CPUs   Sched     Active   Domain count
Pool-0               8    credit       y          4
----

Is $nr_cpupools not therefore 2 when there is a single pool? (2 "\n", one
after the header, one after the data)

> +
> +  logm("Found $nr_cpupools cpupools");
> +  die "There already is more than one cpu pool!" if $nr_cpupools > 1;
> +  die "Non-default cpupool configuration detected"
> +      unless $cppinfo =~ /$default_pool/;

This won't barf on e.g. "Pool-01". Some use of \b might help.

> +
> +  die "This test is meant for xl only"
> +      if toolstack($ho)->{Name} ne "xl";
> +}
> +
> +# Odd pCPUs will end up in out test pool

s/out/our/

> +sub prep_cpulist () {
> +  @cpulist = grep { $_ % 2 } (0..$nr_cpus);
> +  logm("Using the following cpus fo the test pool: @cpulist");

s/fo/for/

> +}
> +
> +sub prep_pool ($) {
> +  my ($sched)= @_;
> +  my @cpustr;
> +
> +  my @cpustr= map { $_ == -1 ? "[ " : $_ == $#cpulist+1 ? " ]" :
> +      "\"$cpulist[$_]\"," } (-1 .. $#cpulist+1);

I think I would write
	my @cpustr = ("[ ".$#cpulist+1." ]");
        push @cpustr, map { "\"$cpulist[$_]\"," } (0 .. $#cpulist+1);
at which point I would realise that the push was something like:
        push @cpustr, map { "\"$_\"," } @cpulist;

I'd also do the "," bit using 
    my $cpustr = join ",", @cpustr;

otherwise you get a trailing "," which you may not want.

(Disclaimer: I'm not 100% sure what output string you are trying to make
here).

> +
> +  target_putfilecontents_stash($ho,100,<<"END","/etc/xen/cpupool-test
> -$sched");
> +name = \"cpupool-test-$sched\"
> +sched = \"$sched\"

Do the quotes really need escaping in this context? I wouldn't have
expected so.

> +cpus = @cpustr
> +END
> +}
> +
> +
> +check();
> +check_cpus();
> +if ($nr_cpus > 1) {

This will go away I hope.

> +  prep_cpulist();
> +  foreach my $s (@schedulers) {
> +    prep_pool("$s");
> +    run("$s");

I think you just want $s, not "$s" in both places. $s is already a string.

> +  }
> +}
>