Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180720110036.32251-1-jolsa@kernel.org> <20180720110036.32251-3-jolsa@kernel.org>
 <20180720145745.GB4329@kernel.org> <20180720151518.GC27176@krava>
In-Reply-To: <20180720151518.GC27176@krava>
From:   Alexander Kapshuk <alexander.kapshuk@gmail.com>
Date:   Fri, 20 Jul 2018 18:22:49 +0300
Message-ID: <CAJ1xhMUeXAwZADxxJAALzwhnMFDQk0hMPeepRML+OLV92fmnJg@mail.gmail.com>
Subject: Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables
To:     jolsa@redhat.com
Cc:     acme@kernel.org, jolsa@kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>, mingo@kernel.org,
        namhyung@kernel.org, dsahern@gmail.com,
        alexander.shishkin@linux.intel.com, a.p.zijlstra@chello.nl
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > The warning message in check_w function uses wrongly
> > > the $file variable instead of $file1 and $file2.
> >
> > Humm,
> >
> > Before:
> >
> > Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'
> >
> > After:
> >
> > Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'
> >
> >
> > The previous version is better, I can then just use:
> >
> > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h
> >
> > and get what changed, with your change I have to go to tools/perf before
> > doing that diff, which is an unnecessary extra step in at least my
> > workflow.
>
> so all paths output based in kernel tree root then, will change
>
> jirka

I was going to ask about this in a separate email initially, but then
thought I'd use this email exchange instead, as my question is about
the code in question. Hope you don't mind.

If I'm reading this right, the intended behavoir of the block of code
below is to test file2 for existance, and if it exists, to evaluate $cmd.
If file1 and file2 are found to differ, print the warning.

test -f $file2 &&
  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
differs from latest version at '$file'" >&2

The '||' path of execution is however also taken if file2 doesn't exist,
which is probably very unlikely to happen. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

Is this something you would rather leave as is, or perhaps use something
along the lines of the code below instead:

test -f $file2 && {
eval $cmd ||
echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
version at '$file'" >&2
}

Thanks.