Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201230110935.582332-1-jolsa@kernel.org>
In-Reply-To: <20201230110935.582332-1-jolsa@kernel.org>
From:   Stephane Eranian <eranian@google.com>
Date:   Tue, 5 Jan 2021 17:33:38 -0800
Message-ID: <CABPqkBTQfFWvEba-=T6ms=GTsjrZUosRQyZZK-EMZ2c_2NQvAA@mail.gmail.com>
Subject: Re: [PATCHv2] perf tools: Detect when pipe is passed as perf data
To:     Jiri Olsa <jolsa@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Ingo Molnar <mingo@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Michael Petlan <mpetlan@redhat.com>,
        Ian Rogers <irogers@google.com>,
        Alexei Budankov <abudankov@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Wed, Dec 30, 2020 at 3:09 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently we allow pipe input/output only through '-' string
> being passed to '-o' or '-i' options, like:
>
It seems to me it would be useful to auto-detect that the perf.data
file is in pipe vs. file mode format.
Your patch detects the type of the file which is something different
from the format of its content.
Thanks.

>   # mkfifo perf.pipe
>   # perf record --no-buffering -e 'sched:sched_switch' -o - > perf.pipe &
>   [1] 354406
>   # cat perf.pipe | ./perf --no-pager script -i - | head -3
>             perf 354406 [000] 168190.164921: sched:sched_switch: perf:354406..
>      migration/0    12 [000] 168190.164928: sched:sched_switch: migration/0:..
>             perf 354406 [001] 168190.164981: sched:sched_switch: perf:354406..
>   ...
>
> This patch detects if given path is pipe and set the perf data
> object accordingly, so it's possible now to do above with:
>
>   # mkfifo perf.pipe
>   # perf record --no-buffering -e 'sched:sched_switch' -o perf.pipe &
>   [1] 360188
>   # perf --no-pager script -i ./perf.pipe | head -3
>             perf 354442 [000] 168275.464895: sched:sched_switch: perf:354442..
>      migration/0    12 [000] 168275.464902: sched:sched_switch: migration/0:..
>             perf 354442 [001] 168275.464953: sched:sched_switch: perf:354442..
>
> It's of course possible to combine any of above ways.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> v2:
>   - removed O_CREAT|O_TRUNC flags from pipe's write end
>
>  tools/perf/util/data.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index f29af4fc3d09..4dfa9e0f2fec 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -159,7 +159,7 @@ int perf_data__update_dir(struct perf_data *data)
>         return 0;
>  }
>
> -static bool check_pipe(struct perf_data *data)
> +static int check_pipe(struct perf_data *data)
>  {
>         struct stat st;
>         bool is_pipe = false;
> @@ -172,6 +172,15 @@ static bool check_pipe(struct perf_data *data)
>         } else {
>                 if (!strcmp(data->path, "-"))
>                         is_pipe = true;
> +               else if (!stat(data->path, &st) && S_ISFIFO(st.st_mode)) {
> +                       int flags = perf_data__is_read(data) ?
> +                                   O_RDONLY : O_WRONLY;
> +
> +                       fd = open(data->path, flags);
> +                       if (fd < 0)
> +                               return -EINVAL;
> +                       is_pipe = true;
> +               }
>         }
>
>         if (is_pipe) {
> @@ -190,7 +199,8 @@ static bool check_pipe(struct perf_data *data)
>                 }
>         }
>
> -       return data->is_pipe = is_pipe;
> +       data->is_pipe = is_pipe;
> +       return 0;
>  }
>
>  static int check_backup(struct perf_data *data)
> @@ -344,8 +354,11 @@ static int open_dir(struct perf_data *data)
>
>  int perf_data__open(struct perf_data *data)
>  {
> -       if (check_pipe(data))
> -               return 0;
> +       int err;
> +
> +       err = check_pipe(data);
> +       if (err || data->is_pipe)
> +               return err;
>
>         /* currently it allows stdio for pipe only */
>         data->use_stdio = false;
> @@ -410,8 +423,10 @@ int perf_data__switch(struct perf_data *data,
>  {
>         int ret;
>
> -       if (check_pipe(data))
> -               return -EINVAL;
> +       ret = check_pipe(data);
> +       if (ret || data->is_pipe)
> +               return ret;
> +
>         if (perf_data__is_read(data))
>                 return -EINVAL;
>
> --
> 2.26.2
>