From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH OSSTEST 5/5 v2] mg-show-flight-runvars:
	recurse on buildjobs upon request
Date: Wed, 3 Feb 2016 12:01:37 +0000
Message-ID: <22193.60449.317320.315994@mariner.uk.xensource.com>
References: <1454336912-28503-5-git-send-email-ian.campbell@citrix.com>
	<1454492932-24427-1-git-send-email-ian.campbell@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1454492932-24427-1-git-send-email-ian.campbell@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <ian.campbell@citrix.com>
Cc: xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

Ian Campbell writes ("[PATCH OSSTEST 5/5 v2] 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.
> 
> The output is resorted by runvar name which is the desired default
> behaviour. As usual can be piped to sort(1) to sort by flight+job.
...
> +if ($recurse) {
> +    foreach my $row (@rows) {
> +	next unless $row->[1] =~ m/^(?:.*_)?([^_]*)buildjob$/;
> +	next if $jobs{$row->[2]};

This does the deduping based on $row->[2] which is the runvar value.
But the same job might be referred to by different runvar values.  I
think this means that you might report the same job twice.

If
   1234.foo  buildjob   wombat
   1234.bar  buildjob   1234.wombat
then you dump 1234.wombat twice.

> +	my ($oflight, $ojob) = flight_otherjob($tflight, $row->[2]);

Also the place you set $jobs{} is somewhat wrong.  If there is a job
with no rows you can process it twice (harmlessly, I think, as it
happens).


So I think you want
        next if $jobs{$oflight,$ojob}++;

> +	collect($oflight, "job = ?", $ojob);
> +
> +	# collect() appends to @rows, so we don't need any special
> +	# handling to pickup anything which was newly added.

I don't think this is true.  Or at least, it happens to be true, but
my copy of the FM says:

       If any part of LIST is an array, "foreach" will get very
       confused if you add or remove elements within the loop body,
       for example with "splice".  So don't do that.

AIUI you're avoiding actual recursion, inside collect()'s row loop, to
avoid trying to have one SQL query for each nesting level.  It might
be worth mentioning that somewhere.

Maybe you should have the foreach use an explicit row index.

Ian.