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;
Date:   Thu, 1 Apr 2021 15:18:04 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Nicholas Fraser <nfraser@codeweavers.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Stephane Eranian <eranian@google.com>,
        Tan Xiaojun <tanxiaojun@huawei.com>,
        Jin Yao <yao.jin@linux.intel.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf data: Add JSON export
Message-ID: <YGXIDGfCLVtFuxgT@krava>
References: <c4d0b0f1-79f4-f08d-3d7e-00046120f845@codeweavers.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c4d0b0f1-79f4-f08d-3d7e-00046120f845@codeweavers.com>
Precedence: bulk

On Wed, Mar 31, 2021 at 06:42:48AM -0400, Nicholas Fraser wrote:
> From ddcfd620e7cad4100d0076090c4b39dba8aeead3 Mon Sep 17 00:00:00 2001
> From: Nicholas Fraser <nfraser@codeweavers.com>
> Date: Wed, 31 Mar 2021 06:10:00 -0400
> Subject: [PATCH] perf data: Add JSON export

no need to add headers again in here

> 
> This adds a feature to export perf data to JSON. It uses a minimal
> inline JSON encoding, no external dependencies. Currently it only
> outputs some headers and sample metadata but it's easily extensible.
> 
> Use it like this:
> 
>     perf data convert --to-json out.json

please add similar output summary message we have for CTF conversion:

	[ perf data convert: Converted 'perf.data' into CTF data 'data' ]
	[ perf data convert: Converted and wrote 0.000 MB (10 samples) ]

also I will not push hard for test, becase we don't have any for CTF ;-)
but if you could think of any, that'd be great

> +
> +static void output_headers(struct perf_session *session, struct convert_json *c)
> +{
> +	struct stat st;
> +	struct perf_header *header = &session->header;
> +	int ret;
> +	int fd = perf_data__fd(session->data);
> +	int i;
> +	bool first;
> +
> +	fprintf(c->out, "\n\t\t\t\"header-version\": %u", header->version);
> +
> +	ret = fstat(fd, &st);
> +	if (ret >= 0) {
> +		time_t stctime = st.st_mtime;
> +		char buf[256];
> +
> +		strftime(buf, sizeof(buf), "%FT%TZ", gmtime(&stctime));
> +		fprintf(c->out, ",\n\t\t\t\"captured-on\": \"%s\"", buf);
> +	} else {
> +		pr_debug("Failed to get mtime of source file, not writing \"captured-on\"");
> +	}
> +
> +	fprintf(c->out, ",\n\t\t\t\"data-offset\": %" PRIu64, header->data_offset);
> +	fprintf(c->out, ",\n\t\t\t\"data-size\": %" PRIu64, header->data_size);
> +	fprintf(c->out, ",\n\t\t\t\"feat-offset\": %" PRIu64, header->feat_offset);

I was wondering how to make this \t mess more readable,
how about you define function like output_json:

	output_json(FILE, level, field, format, ...);

and use it:

	output_json(c->out, 3, "data-offset", "PRIu64", header->data_offset);
	output_json(c->out, 3, "data-size", "PRIu64", header->data_size);
	output_json(c->out, 3, "feat-offset", PRIu64, header->feat_offset);

similar way as we do for pr_debug -> eprintf

SNIP

> +
> +	fd = open(output_name, O_CREAT | O_WRONLY | (opts->force ? 0 : O_EXCL), 0666);
> +	if (fd == -1) {
> +		if (errno == EEXIST)
> +			pr_err("Output file exists. Use --force to overwrite it.\n");
> +		else
> +			pr_err("Error opening output file!\n");
> +		return -1;
> +	}
> +
> +	c.out = fdopen(fd, "w");
> +	if (!c.out) {
> +		fprintf(stderr, "Error opening output file!\n");
> +		return -1;
> +	}
> +
> +	session = perf_session__new(&data, false, &c.tool);
> +	if (IS_ERR(session)) {
> +		fprintf(stderr, "Error creating perf session!\n");
> +		return -1;

here we should close c.out and call perf_session__delete,
we normaly do goto to the end of the function in this case

> +	}
> +
> +	if (symbol__init(&session->header.env) < 0) {
> +		fprintf(stderr, "Symbol init error!\n");
> +		return -1;
> +	}
> +
> +	// Version number for future-proofing. Most additions should be able to be
> +	// done in a backwards-compatible way so this should only need to be bumped
> +	// if some major breaking change must be made.
> +	fprintf(c.out, "{\n\t\"linux-perf-json-version\": 1,");
> +
> +	// Output headers
> +	fprintf(c.out, "\n\t\"headers\": {");
> +	output_headers(session, &c);
> +	fprintf(c.out, "\n\t},");
> +
> +	// Output samples
> +	fprintf(c.out, "\n\t\"samples\": [");
> +	perf_session__process_events(session);
> +	fprintf(c.out, "\n\t]\n}\n");
> +

you need to close c.out

> +	perf_session__delete(session);
> +	return 0;
> +}
> diff --git a/tools/perf/util/data-convert.h b/tools/perf/util/data-convert.h
> index feab5f114e37..1b4c5f598415 100644
> --- a/tools/perf/util/data-convert.h
> +++ b/tools/perf/util/data-convert.h
> @@ -2,10 +2,20 @@
>  #ifndef __DATA_CONVERT_H
>  #define __DATA_CONVERT_H
>  
> +#include <stdbool.h>
> +
>  struct perf_data_convert_opts {
>  	bool force;
>  	bool all;
>  	bool tod;
>  };
>  
> +#ifdef HAVE_LIBBABELTRACE_SUPPORT
> +int bt_convert__perf2ctf(const char *input_name, const char *to_ctf,
> +			 struct perf_data_convert_opts *opts);
> +#endif /* HAVE_LIBBABELTRACE_SUPPORT */
> +
> +int bt_convert__perf2json(const char *input_name, const char *to_ctf,
> +			 struct perf_data_convert_opts *opts);
> +
>  #endif /* __DATA_CONVERT_H */

great, thanks for this
jirka