linux-kernel - Re: [PATCH] perf, tools: Make build fail on JSON parse error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 24 Jul 2017 12:07:55 -0700
From:   Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
To:     Andi Kleen <andi@...stfloor.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>, jolsa@...nel.org,
        linux-kernel@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH] perf, tools: Make build fail on JSON parse error

Andi Kleen [andi@...stfloor.org] wrote:
> On Mon, Jul 24, 2017 at 02:42:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jul 24, 2017 at 04:16:49PM +0200, Jiri Olsa escreveu:
> > > On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> > > > Andi Kleen [andi@...stfloor.org] wrote:
> > > > > From: Andi Kleen <ak@...ux.intel.com>
> > > > > 
> > > > > Today, when a JSON file fails parsing the build continues,
> > > > > but there are no json files built in, which is difficult to debug later.
> > > > > Make the build stop on a parse error instead.
> > > > 
> > > > I see the problem and we were being defensive to not break the build
> > > > on architectures that don't yet have the PMU event lists. It will be
> > > > good to check build on an architecture other than x86/powerpc.
> > > > 
> > > > Also, following comments may no longer be applicable?
> > > 
> > > Isn't the Andi's change only to fail in case there's
> > > a real error in process_one_file? I think you can still
> > > have empty events dir.
> > 
> > That explains why all the cross builds failed when I added that cset?
> 
> Hmm, let me test. It was supposed to only fail when there
> is a file, but it cannot be parsed.

Wonder if adding this check for event lists helps.

---
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index baa073f..4de5c87 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -840,6 +840,7 @@ int main(int argc, char *argv[])
 	const char *arch;
 	const char *output_file;
 	const char *start_dirname;
+	struct stat stbuf;
 
 	prog = basename(argv[0]);
 	if (argc < 4) {
@@ -861,11 +862,17 @@ int main(int argc, char *argv[])
 		return 2;
 	}
 
+	sprintf(ldirname, "%s/%s", start_dirname, arch);
+
+	/* If architecture does not have any event lists, bail out */
+	if (stat(ldirname, &stbuf) < 0) {
+		pr_info("%s: Arch %s has no PMU event lists\n", prog, arch);
+		goto empty_map;
+	}
+
 	/* Include pmu-events.h first */
 	fprintf(eventsfp, "#include \"../../pmu-events/pmu-events.h\"\n");
 
-	sprintf(ldirname, "%s/%s", start_dirname, arch);
-
 	/*
 	 * The mapfile allows multiple CPUids to point to the same JSON file,
 	 * so, not sure if there is a need for symlinks within the pmu-events
@@ -882,6 +889,9 @@ int main(int argc, char *argv[])
 	if (rc && verbose) {
 		pr_info("%s: Error walking file tree %s\n", prog, ldirname);
 		goto empty_map;
+	} else if (rc < 0) {
+		/* Make build fail */
+		return 1;
 	} else if (rc) {
 		goto empty_map;
 	}
@@ -896,7 +906,8 @@ int main(int argc, char *argv[])
 
 	if (process_mapfile(eventsfp, mapfile)) {
 		pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
-		goto empty_map;
+		/* Make build fail */
+		return 1;
 	}
 
 	return 0;