| Message ID | 20200310134635.99810-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [OSSTEST,v2,1/2] examine: detect IOMMU availability and add it as a hostflag | expand |
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 ?
> + 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 ?
> +++ 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".
Apart from these things, the code all LGTM.
Thanks,
Ian.
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.
Roger Pau Monne writes ("Re: [PATCH OSSTEST v2 1/2] examine: detect IOMMU availability and add it as a hostflag"):
> On Tue, Mar 10, 2020 at 03:51:34PM +0000, Ian Jackson wrote:
> > 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.
I prefer that to the alternative of mixing it up.
> [rest snipped]
OK, good.
Thanks,
Ian.
diff --git a/Osstest/HostDB/Executive.pm b/Osstest/HostDB/Executive.pm index 7ffca6c4..8bedbdff 100644 --- a/Osstest/HostDB/Executive.pm +++ b/Osstest/HostDB/Executive.pm @@ -90,6 +90,25 @@ END return $flags; } +sub set_flag($$$) { + my ($hd, $ho, $flag) = @_; + 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"; + + db_retry($dbh_tests, [qw(resources)], sub { + $rmq->execute($ho->{Name}, $flag); + $addq->execute($ho->{Name}, $flag); + }); +} + sub get_arch_platforms ($$$) { my ($hd, $blessing, $arch, $suite) = @_; diff --git a/Osstest/HostDB/Static.pm b/Osstest/HostDB/Static.pm index 0c6be3ee..8ac39cf5 100644 --- a/Osstest/HostDB/Static.pm +++ b/Osstest/HostDB/Static.pm @@ -72,6 +72,12 @@ sub get_flags ($$) { #method return $flags; } +sub set_property($$$) { + my ($hd, $ho, $flag) = @_; + + die "Cannot set flags in standalone mode for $ho->{Name} $flag\n"; +} + sub get_arch_platforms ($$$) { my ($hd, $blessing, $arch, $suite) = @_; diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm index f49ed529..0291dbd8 100644 --- a/Osstest/TestSupport.pm +++ b/Osstest/TestSupport.pm @@ -84,9 +84,9 @@ BEGIN { get_target_property get_host_native_linux_console hostnamepath hostnamepath_list set_runtime_hostflag power_state power_cycle power_reboot_attempts - serial_fetch_logs set_host_property + serial_fetch_logs set_host_property set_host_flag propname_massage propname_check - hostprop_putative_record + hostprop_putative_record hostflag_putative_record get_stashed open_unique_stashfile compress_stashed dir_identify_vcs @@ -1411,6 +1411,18 @@ sub hostprop_putative_record ($$$) { store_runvar("hostprop/$ho->{Ident}/$prop", $val); } +sub set_host_flag ($$) { + my ($ho,$flag) = @_; + + $mhostdb->set_flag($ho, $flag); +} + +sub hostflag_putative_record ($$) { + my ($ho, $prop) = @_; + + store_runvar("hostflag/$ho->{Ident}/$prop", ""); +} + sub get_target_property ($$;$); sub get_target_property ($$;$) { my ($ho, $prop, $defval) = @_; diff --git a/sg-run-job b/sg-run-job index 7c58d4ba..f6bfdfd5 100755 --- a/sg-run-job +++ b/sg-run-job @@ -679,6 +679,7 @@ proc examine-host-examine {install} { if {$ok} { run-ts -. = ts-examine-serial-post + host run-ts . = ts-examine-logs-save + host + run-ts . = ts-examine-iommu + host run-ts . = ts-examine-hostprops-save } } diff --git a/ts-examine-hostprops-save b/ts-examine-hostprops-save index 55d23392..502c2db2 100755 --- a/ts-examine-hostprops-save +++ b/ts-examine-hostprops-save @@ -27,20 +27,25 @@ tsreadconfig(); our $blessing = intended_blessing(); -logm("setting host properties"); +logm("setting host properties and flags"); # NB: in order to aid debug only attempt to save the host props on flights # with intended real blessing, for the rest just do a dry run. our $dry_run = $blessing ne "real"; -logm("not saving host props with intended blessing $blessing != real") +logm("not saving host props/flags with intended blessing $blessing != real") if $dry_run; foreach my $k (sort keys %r) { - next unless $k =~ m/^hostprop\/([^\/]*)\/([^\/]*)$/; - my $ho = selecthost($1); - my $prop = $2; - - logm("recording for $ho->{Name} $prop=$r{$k}"); - - set_host_property($ho, $prop, $r{$k}) if !$dry_run; + next unless $k =~ m/^host(prop|flag)\/([^\/]*)\/([^\/]*)$/; + my $type = $1; + my $ho = selecthost($2); + my $prop = $3; + + if ($type == "flag") { + logm("recording flag $prop for $ho->{Name}"); + set_host_flag($ho, $prop) if !$dry_run; + } else { + logm("recording prop for $ho->{Name} $prop=$r{$k}"); + set_host_property($ho, $prop, $r{$k}) if !$dry_run; + } } 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 +# This is part of "osstest", an automated testing framework for Xen. +# Copyright (C) 2009-2020 Citrix Inc. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +use strict qw(vars); +BEGIN { unshift @INC, qw(.); } +use Osstest; +use Osstest::TestSupport; + +tsreadconfig(); + +our ($whhost) = @ARGV; +$whhost ||= 'host'; +our $ho= selecthost($whhost); + +our $has_iommu = !target_cmd_root_status($ho, 'xl info|grep directio', 10); +logm("$ho->{Ident} iommu: $has_iommu"); +hostflag_putative_record($ho, "iommu") if $has_iommu;
Introduce a new test to check for iommu availability and add it as a hostflag if found. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Osstest/HostDB/Executive.pm | 19 +++++++++++++++++++ Osstest/HostDB/Static.pm | 6 ++++++ Osstest/TestSupport.pm | 16 ++++++++++++++-- sg-run-job | 1 + ts-examine-hostprops-save | 23 ++++++++++++++--------- ts-examine-iommu | 31 +++++++++++++++++++++++++++++++ 6 files changed, 85 insertions(+), 11 deletions(-) create mode 100755 ts-examine-iommu