Re: [Xen-devel] [PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status to report job stats

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status 
to report job stats"):
> Since this new op is not a change adjust the doc comment accordingly.

Good idea.  (I have already introduced another op which is not a
change: jobs-list.)

> While there add a doc string for the undocumented "branch" subcommand
> of cs-adjust-flight.

Clearly that should have been called branch-set.  Oh well.  (Feel free
to rename it if you feel like it.  I would do so in my tree but
conflicts.)

> +sub change__job_status {
> +    die unless @changes >= 1;
> +    my $jobs = shift @changes;
> +
> +    my $q = $dbh_tests->prepare(<<END);
> +        SELECT status
> +          FROM jobs
> +         WHERE flight = ? AND job = ?
> +END
> +    for_jobs($dstflight, $jobs, sub {
> +     my ($job) = @_;
> +     $q->execute($dstflight, $job);
> +     my ($s) = $q->fetchrow_array();
> +     print "$job $s\n";

 or die $!;

> +    });
> +}

This output format is awkward, isn't it ?

Would it be too horrible to omit `$job ' if the spec was a literal and
can therefore only match one item ?  You can test that with something like
    print "$job " or die $! if defined spec_re($jobs);

> +    case "$status" in
> +     $job\ *) echo ${status#$job };;

If you decide to keep `$job ' in the output, should read:

  +     "$job"\ *) echo ${status#$job };;

since otherwise wildcards in $job would be treated as an attempt to do
glob matching.

Or, better

  +     "$job "*) echo ${status#$job };;

which is a better idiom.

> +     *)
> +         echo >&2 "ERROR: $status"
> +         exit 1

Do we not have `fail' in scope ?  Also TBH I'm not sure you need to be
this careful anyway...

> +     if [ $# -ne 1 ] ; then
> +         echo "get-job-status: Need job" >&2
> +         exit 1

`fail' again ?

Ian.

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