Re: [Xen-devel] [PATCH OSSTEST v1 5/5] mg-show-flight-runvars: recurse on buildjobs upon request

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2016-02-01 at 15:19 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v1 5/5] mg-show-flight-runvars:
> recurse on buildjobs upon request"):
> > By looping over @rows looking for buildjobs runvars and adding those
> > jobs to the output until nothing changes.
> ...
> > Note that synth runvars (if requests) are now sorted in with the
> > regular ones, whereas previously they were listed last. Retaining the
> > old behaviour would be too complex I feel.
> 
> ...
> > -sub collect ($) {
> > -ÂÂÂÂmy ($flight) = @_;
> > +ÂÂÂÂ$jobcond //= "TRUE";
> ...
> > ÂÂÂÂÂmy $q = $dbh_tests->prepare
> > Â   ("SELECT synth, ".(join ',', @cols)." $qfrom ORDER BY synth,
> > name, job");
> 
> You probably want to drop the `synth' from the ORDER here, even if you
> take my suggestion below.ÂÂThe sort is still a good idea here because
> it ensures that the output is deterministic.

OK

> > +ÂÂÂÂmy ($oflight, $ojob) = flight_otherjob($tflight, $row->[2]);
> > +ÂÂÂÂcollect($oflight, "job = '$ojob'");
> 
> SQL injection vulnerability, I think.ÂÂ(There are lots of places
> where jobs are treated this way, but they come from the command line
> or similar, or have been checked against some regexp.)
> 
> I think you should use the $jobcond @jobcondparams pattern.

Indeed. Fixed.

> -foreach my $row (@rows) {
> > +# Sort by runvar name, then (flight+)job.
> > +foreach my $row (sort { $a->[1] cmp $b->[1]//$a->[0] cmp $b->[1] }
> > @rows) {
> 
> If you retain it this way, put spaces round your //.
> 
> But maybe you should use a Schwartzian transform instead.
> 
> ÂÂÂmy $xform = sub { [ ($->[1] =~ m/\~$/)." $_->[1] $_->[0]", $_ ]; };

> ÂÂÂforeach my $row (map { $_->[1] } sort { $xform->($a) cmp
> ÂÂÂÂÂÂÂ$xform->($b) etc.

ITIYM:

 Âforeach my $row (map { $_->[1] }
          sort { $a->[0] cmp $b->[0] }
          map { $xform->($_) }
          @rows) {
    ...
 Â}

Since doing the xform-> in the sort defeats the purpose (or I don't
properly understand the Schwartzian transform)

> This makes the synth sorting easy too.

Right.



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