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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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.

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