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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 10, 2015 at 12:10:41PM +0100, Ian Jackson wrote:
> 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) {
> 

Ack.

> 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 .= "
>     }
> 

Ack.

> 
> > +    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.
> 

That's easy.

sub guest_var_boolean ($$) {
    my ($gho, $runvartail) = @_;
    return guest_var($gho, $runvartail, 'false') ~= m/true/;
}

> 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 ?
> 

No. It's a blank line after the closing "}". Not sure why it showed up
that way.

> >      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.
> 

No problem.

Wei.

> Ian.

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