Re: [OSSTEST PATCH 08/13] Reporting: Break out report_blessingscond

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

On Mon, 2015-06-29 at 18:14 +0100, Ian Jackson wrote:
> The returned SQL is a self-contained expression, and does not require
> additional bind parameters.  To spot SQL quoting problems, we die if
> a blessing is not reasonable.
> 
> The use sites of the $blessingcond in sg-report-flight are adjusted to
> no longer pass @blessings to execute.
> 
> No overall functional change with reasonable blessings.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

BTW, I noticed that your valid blessing regex here includes "-", which
is not allowed elsewhere, which breaks "./standalone make-flight -f
$branch $branch" for our common branch names meaning the flight name
needs some munging. A minor inconvenience, but if this check were moved
to a centralised helper it could be easily fixed...

Or at least this was once the case, I can't see the problematic check
right now and haven't actually tried this for a while. Perhaps it got
fixed already and I didn't notice?

> ---
>  Osstest/Executive.pm |   13 +++++++++++++
>  sg-report-flight     |   10 +++-------
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
> index 9df4d91..ba668bc 100644
> --- a/Osstest/Executive.pm
> +++ b/Osstest/Executive.pm
> @@ -47,6 +47,7 @@ BEGIN {
>      @EXPORT      = qw(get_harness_rev grabrepolock_reexec
>                        findtask @all_lock_tables
>                        report_run_getinfo report_altcolour
> +                      report_blessingscond
>                        tcpconnect_queuedaemon plan_search
>                        alloc_resources alloc_resources_rollback_begin_work
>                        resource_check_allocated resource_shared_mark_ready
> @@ -246,6 +247,18 @@ sub report_altcolour ($) {
>      return "bgcolor=\"#".(qw(d0d0d0 ffffff))[$bool]."\"";
>  }
>  
> +sub report_blessingscond ($$) {
> +    my ($blessings, $maxflight) = @_;
> +    my $blessingscond= '('.join(' OR ', map {
> +	die if m/[^-_.0-9a-z]/;
> +	"blessing='$_'"
> +				} @$blessings).')';
> +    if (defined $maxflight) {
> +	$blessingscond= "( flight <= $maxflight AND $blessingscond )";
> +    }
> +    return $blessingscond;
> +}
> +
>  #---------- host (and other resource) allocation ----------
>  
>  our $taskid;
> diff --git a/sg-report-flight b/sg-report-flight
> index 117b609..c1661ec 100755
> --- a/sg-report-flight
> +++ b/sg-report-flight
> @@ -127,11 +127,7 @@ our $cw= 79;
>  our $tl= 20;
>  our $htmlleaf= "info.html";
>  
> -our $blessingscond= '('.join(' OR ', map { "blessing=?" } @blessings).')';
> -
> -if (defined $maxflight) {
> -    $blessingscond= "( flight <= $maxflight AND $blessingscond )";
> -}
> +our $blessingscond= report_blessingscond(\@blessings, $maxflight);
>  
>  sub displayflightnum ($) {
>      my ($flight) = @_;
> @@ -202,7 +198,7 @@ END
>        ORDER BY blessing ASC, flight DESC
>  END
>      $flightsq= db_prepare($flightsq);
> -    $flightsq->execute(@flightsq_params, @blessings);
> +    $flightsq->execute(@flightsq_params);
>  
>      my $buildflightsq= db_prepare(<<END);
>          SELECT val FROM runvars
> @@ -691,7 +687,7 @@ END
>              next;
>          }
>  
> -        $anypassq->execute($j->{job}, $s->{testid}, @blessings);
> +        $anypassq->execute($j->{job}, $s->{testid});
>          if (!$anypassq->fetchrow_hashref()) {
>              print MRO "never-passed $j->{job} $s->{testid} $st\n";
>              print DEBUG " never passed\n";