Re: [Xen-devel] [PATCH OSSTEST v2 1/2] examine: detect IOMMU availability and add it as a hostflag

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 10, 2020 at 03:51:34PM +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH OSSTEST v2 1/2] examine: detect IOMMU 
> availability and add it as a hostflag"):
> > Introduce a new test to check for iommu availability and add it as a
> > hostflag if found.
> 
> Thanks.
> 
> > +sub set_flag($$$) {
> 
> Firstly, can you break this new code out into its own patch ?

Can do, but then there will be no user of the introduced code which I
tend to avoid.

> > +    my ($hd, $ho, $flag) = @_;
> 
> Secondly, this doesn't take a booleanish for yes/no.  I think it
> should.  Ie, it should be capable of both setting and clearing.
> I think leaving that functionality out now is too close to Extreme
> Programming for my taste, even for osstest :-).
> 
> > +    my $rmq = $dbh_tests->prepare(<<END);
> > +        DELETE FROM hostflags WHERE hostname=? AND hostflag=?
> > +END
> > +    my $addq = $dbh_tests->prepare(<<END);
> > +        INSERT INTO hostflags (hostname,hostflag) VALUES (?,?)
> > +END
> > +    my $blessing = intended_blessing();
> > +
> > +    die "Attempting to modify host flags with intended blessing $blessing 
> > != real"
> > +        if $blessing ne "real";
> 
> Much of this code is identical to that in set_property.
> I think maybe you could introduce
> 
>    sub modify_host ($$$) {
>        my ($hd, $ho, $fn) = @_;
> 
> which contains the call to intended_blessing and passes its $fn
> argument to db_retry ?

Ack.

> 
> > +++ b/Osstest/HostDB/Static.pm
> ...
> > +sub set_property($$$) {
> > +    my ($hd, $ho, $flag) = @_;
> > +
> > +    die "Cannot set flags in standalone mode for $ho->{Name} $flag\n";
> > +}
> 
> I considered whether this meant that modify_host ought to be part of
> some base class but $rmq etc. would need setting up and plumbing
> through so that seems both too annoying and to make things too
> abstract.  But if you think you can and would like to, please do...
> 
> > diff --git a/ts-examine-iommu b/ts-examine-iommu
> > new file mode 100755
> > index 00000000..ae91d4d2
> > --- /dev/null
> > +++ b/ts-examine-iommu
> > @@ -0,0 +1,31 @@
> > +#!/usr/bin/perl -w
> ...
> > +our $has_iommu = !target_cmd_root_status($ho, 'xl info|grep directio', 10);
> 
> Please fetch the output of xl info and do the grepping in perl.
> 
> The way you do it here will miss a situation where xl info fails
> completely, which ought to be fatal, not treated as "no iommu".

Sure.

> Apart from these things, the code all LGTM.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel