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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
