Re: [Xen-devel] [PATCH OSSTEST v3] Stubdom test case

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[PATCH OSSTEST v3] Stubdom test case"):
> Currently only QEMU traditional supports stubdom, so we only create
> 
> test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64
> test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm
> test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64
> test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm
...


Thanks.  This is mostly good but I have some quibbles.


The first is that the handling of boolean values is rather odd and
un-idiomatic.

> +    my $stubdom = $xopts{Stubdom};
> +    if (defined $stubdom) {
> +     $cfg .= "device_model_stubdomain_override=$stubdom\n";
> +    }
> +    if (defined $stubdom && $stubdom == 1) {
> +     $cfg .= "serial='pty'";
> +    } else {
> +     $cfg .= "serial='file:/dev/stderr'";
> +    }

`undef' is a perfectly legal boolean false, and in Perl it is not
normal to compare booleanish values with 1 explicitly.  So for the
second if you probably meant simply
    if ($stubdom) {

Also, this code attempts to make a distinction between:
   * stubdom set explicitly to true
   * stubdom set explicitly to false
   * stubdom not set, do whatever the toolstack default is

But actually the latter case will not work unless the default is
non-stubdom, because of the serial=.  So maybe for now it would be
simpler to just have:
    if ($stubdom) {
        $cfg .= "device_model_stubdomain_override=1\n";
        $cfg .= "serial='pty'";
    } else {
        $cfg .= "serial='file:/dev/stderr'";
        $cfg .= "
    }


> +    if (defined $r{enable_stubdom}) {
> +     $enable_stubdom = ($r{enable_stubdom}//'false') =~ m/true/ ? 1 : 0;

I realise we have already acquired an instance of this idiom, but "? 1
: 0" is not very sensible.  It is not usual (in Perl) to explicitly
canonicalise booleans unless necessary - and here, it isn't necessary
(if you get rid of the == 1 above).


This direct use of a global runvar is not right.  You should be using
guest_var.  If you use guest_var then you get to supply the default
more explicitly.

So I would do:
   $stubdom = guest_var($gho,'stubdom','false') =~ m/true/;

I'm not a great fan of this open-coded m/true/ everywhere but I won't
insist you drain that swamp.  But if you felt like creating a
guest_var_boolean to use here, that would be nice.

If you do that then you end up with
   $stubdom = guest_var_boolean($gho,'stubdom')
(assuming guest_var_boolean returns undef for unset runvar if no
default supplied).

That short enough that you can just write it explicitly in
more_prepareguest_hvm (which has access to $gho) and you therefore
don't need to introduce a new Stubdom entry in xopts.

>      }
>  
> +
>      $xopts{VifType} ||= "ioemu";

Whitespace error ?

>      for xsm in $xsms ; do
> -      do_hvm_debian_test_one debianhvm rombios $xsm
> +      for stubdom in true "" ; do
> +        do_hvm_debian_test_one debianhvm rombios $xsm $stubdom
> +      done

My understanding is that we are intending to do away with most of the
non-xsm tests.  Ian, do you agree ?

In which case we should be creating just the two xsm jobs.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel