Re: [Xen-devel] [PATCH OSSTEST v2 5/5] ms-flights-summary: Produce an HTML report of all active flights

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH OSSTEST v2 5/5] ms-flights-summary: Produce an 
HTML report of all active flights"):
> This could surely use better Perl and produce better output, however
> I'm sending it now because it would be useful for further development
> if some or all of the preceding patches could go into production and
> this serves as an example of why I think I want them.

I think it's pretty good actually.  I have some minor stylistic
comments.  I haven't inspected the output, but as you say we can
improve it later.


> +         my $flightinfo= $dbh_tests->selectrow_hashref(<<END);
> +         SELECT started,blessing,branch,intended FROM flights
> +             WHERE flight=$f
> +END

If you indent this by another 4, it would be a bit easier to read.

> +            $flights{$f}= { Nr => $f,
> +                         Stats => {},
> +                         Jobs => {} };

This layout is rather unidiomatic.

  +            $flights{$f}= {
  +                Nr => $f,
  +                Stats => {},
  +                Jobs => {},
  +            };

if it won't go on one line.  (And there's one later where you have the
{ on the next line indented as if it were a code block, which is
odder.)

> +         if ( $evt->{Job} ) {
> +             my $j;
> +             $evt->{Job} =~ m/^([0-9]+)\.(.*)/ or die;
> +             ($fnum,$j) = ($1,$2);
> +             goto anon unless $flights{$fnum};
> +             $f = $flights{$fnum};
> +             goto anon unless $f->{Jobs}{$j};
> +             $job = $f->{Jobs}{$j};
> +         } else {
> + anon:

Surely you mean

  +             $f = $flights{$fnum};
  +             $job = $f->{Jobs}{$j};
  +         }
  +         if (!$job) {

?

> +my @cols = ("Job",
> +         #"Recipe",
> +         "Status", "Resource", "Scheduled");#, "Start", "End", "Host");

qw() ?

> +sub fmttime($)
> +{

{ should be on previous line, and space before (.

> +#    my $prinfo = sub {
> +#    job_cell($job);
> +#    cell($info->{Status} // "???");
> +#    };
> +
> +#    my $row = sub {
> +#    print "<tr>\n";
> +#    $prinfo->();
> +#    $prinfo = sub { print "<td></td>" foreach (1..2) };
> +#    rawcell($_[0].$_[1]) if $_[1];
> +#    cell(fmt_timespan($_[2])) if $_[2];
> +#    print "</tr>\n";
> +#    };

Are you deliberately leaving this here, commented out ?


> +     while (my ($reso,$rinf) = each %{ $info->{Reso} }) {
> +         row($omitjob, $job, $info->{Status}, $pfx.encode_entities($reso), 
> $rinf)

Line is rather long.


> diff --git a/ms-planner b/ms-planner
> index f38f05b..35d430b 100755
> --- a/ms-planner
> +++ b/ms-planner
> @@ -289,6 +289,7 @@ END
>           $info= "rogue task $arow->{subtask}";
>       }
>       $plan->{Allocations}{$reskey}= {
> +         # Can we find a Job here?
>              Task => $arow->{owntaskid},
>           Info => $info,

I don't understand this comment.

Ian.

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