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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-09-15 at 18:18 +0100, Ian Jackson wrote:
> 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.


My thinking is that this would prevent people using an old style name for
the database prop and getting away with it by making the same error in the
host flags.

Of course this now lets people get away with the wrong name in the host
flags so long as the database is correct.

    die $prop ne propname_massage($prop) 

???

> 
> 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)' ?

Done.

> 
> > +    # 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_.)

I did s/required minimum/maximum minimum/

> > 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+:   ?

Yep, done

[...]
> > +		    or die "get ResourceCondition $@";
> > +
> > +		push @{$hid->{Conds}{host}}, $o;
> 
> I normally like to put some spaces inside the @{ } in this kind of
> thing.

Ack.

> 
> Ian.