[PATCH v1 0/7] Add and use run_command_strbuf

[PATCH v1 0/7] Add and use run_command_strbuf

[Ian Rogers]

It is commonly useful to run a command using "/bin/sh -c" (like popen)
and to place the output in a string. Move strbuf to libapi, add a new
run_command that places output in a strbuf, then use it in help and
llvm in perf. Some small strbuf efficiency improvements are
included. Whilst adding a new function should increase lines-of-code,
by sharing two similar usages in perf llvm and perf help, the overall
lines-of-code is moderately reduced.

First "perf llvm: Fix inadvertent file creation" is cherry-picked
from:
https://lore.kernel.org/lkml/20230105082609.344538-1-irogers@google.com/
to avoid a merge conflict. The next patches deal with moving strbuf,
adding the run_command function with Makefile dependency from
libsubcmd to libapi, and improving the strbuf performance. The final
two patches add usage from the perf command.

Ian Rogers (7):
  perf llvm: Fix inadvertent file creation
  tools lib: Move strbuf to libapi
  tools lib subcmd: Add run_command_strbuf
  tools lib api: Minor strbuf_read improvements
  tools lib api: Tweak strbuf allocation size computation
  perf help: Use run_command_strbuf
  perf llvm: Remove read_from_pipe

 tools/lib/api/Build                   |   1 +
 tools/lib/api/Makefile                |   2 +-
 tools/{perf/util => lib/api}/strbuf.c |  28 ++--
 tools/{perf/util => lib/api}/strbuf.h |   0
 tools/lib/subcmd/Makefile             |  32 +++-
 tools/lib/subcmd/run-command.c        |  30 ++++
 tools/lib/subcmd/run-command.h        |  14 ++
 tools/perf/bench/evlist-open-close.c  |   2 +-
 tools/perf/builtin-help.c             |  49 ++----
 tools/perf/builtin-list.c             |   2 +-
 tools/perf/tests/bpf.c                |  12 +-
 tools/perf/tests/llvm.c               |  18 +--
 tools/perf/tests/llvm.h               |   3 +-
 tools/perf/util/Build                 |   1 -
 tools/perf/util/bpf-loader.c          |   9 +-
 tools/perf/util/cache.h               |   2 +-
 tools/perf/util/dwarf-aux.c           |   2 +-
 tools/perf/util/env.c                 |   2 +-
 tools/perf/util/header.c              |   2 +-
 tools/perf/util/llvm-utils.c          | 207 ++++++++------------------
 tools/perf/util/llvm-utils.h          |   6 +-
 tools/perf/util/metricgroup.c         |   2 +-
 tools/perf/util/pfm.c                 |   2 +-
 tools/perf/util/pmu.c                 |   2 +-
 tools/perf/util/probe-event.c         |   2 +-
 tools/perf/util/probe-file.c          |   2 +-
 tools/perf/util/probe-finder.c        |   2 +-
 tools/perf/util/sort.c                |   2 +-
 28 files changed, 201 insertions(+), 237 deletions(-)
 rename tools/{perf/util => lib/api}/strbuf.c (87%)
 rename tools/{perf/util => lib/api}/strbuf.h (100%)

-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 1/7] perf llvm: Fix inadvertent file creation

[Ian Rogers]

The LLVM template is first echo-ed into command_out and then
command_out executed. The echo surrounds the template with double
quotes, however, the template itself may contain quotes. This is
generally innocuous but in tools/perf/tests/bpf-script-test-prologue.c
we see:
...
SEC("func=null_lseek file->f_mode offset orig")
...
where the first double quote ends the double quote of the echo, then
the > redirects output into a file called f_mode.

To avoid this inadvertent behavior substitute redirects and similar
characters to be ASCII control codes, then substitute the output in
the echo back again.

Fixes: 5eab5a7ee032 ("perf llvm: Display eBPF compiling command in debug output")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/llvm-utils.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/llvm-utils.c b/tools/perf/util/llvm-utils.c
index 650ffe336f3a..4e8e243a6e4b 100644
--- a/tools/perf/util/llvm-utils.c
+++ b/tools/perf/util/llvm-utils.c
@@ -531,14 +531,37 @@ int llvm__compile_bpf(const char *path, void **p_obj_buf,
 
 	pr_debug("llvm compiling command template: %s\n", template);
 
+	/*
+	 * Below, substitute control characters for values that can cause the
+	 * echo to misbehave, then substitute the values back.
+	 */
 	err = -ENOMEM;
-	if (asprintf(&command_echo, "echo -n \"%s\"", template) < 0)
+	if (asprintf(&command_echo, "echo -n \a%s\a", template) < 0)
 		goto errout;
 
+#define SWAP_CHAR(a, b) do { if (*p == a) *p = b; } while (0)
+	for (char *p = command_echo; *p; p++) {
+		SWAP_CHAR('<', '\001');
+		SWAP_CHAR('>', '\002');
+		SWAP_CHAR('"', '\003');
+		SWAP_CHAR('\'', '\004');
+		SWAP_CHAR('|', '\005');
+		SWAP_CHAR('&', '\006');
+		SWAP_CHAR('\a', '"');
+	}
 	err = read_from_pipe(command_echo, (void **) &command_out, NULL);
 	if (err)
 		goto errout;
 
+	for (char *p = command_out; *p; p++) {
+		SWAP_CHAR('\001', '<');
+		SWAP_CHAR('\002', '>');
+		SWAP_CHAR('\003', '"');
+		SWAP_CHAR('\004', '\'');
+		SWAP_CHAR('\005', '|');
+		SWAP_CHAR('\006', '&');
+	}
+#undef SWAP_CHAR
 	pr_debug("llvm compiling command : %s\n", command_out);
 
 	err = read_from_pipe(template, &obj_buf, &obj_buf_sz);
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 2/7] tools lib: Move strbuf to libapi

[Ian Rogers]

Move strbuf, appendable C strings, to libapi so that other libraries
may use it.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/api/Build                   | 1 +
 tools/lib/api/Makefile                | 2 +-
 tools/{perf/util => lib/api}/strbuf.c | 5 +++--
 tools/{perf/util => lib/api}/strbuf.h | 0
 tools/perf/bench/evlist-open-close.c  | 2 +-
 tools/perf/builtin-help.c             | 2 +-
 tools/perf/builtin-list.c             | 2 +-
 tools/perf/util/Build                 | 1 -
 tools/perf/util/cache.h               | 2 +-
 tools/perf/util/dwarf-aux.c           | 2 +-
 tools/perf/util/env.c                 | 2 +-
 tools/perf/util/header.c              | 2 +-
 tools/perf/util/metricgroup.c         | 2 +-
 tools/perf/util/pfm.c                 | 2 +-
 tools/perf/util/pmu.c                 | 2 +-
 tools/perf/util/probe-event.c         | 2 +-
 tools/perf/util/probe-file.c          | 2 +-
 tools/perf/util/probe-finder.c        | 2 +-
 tools/perf/util/sort.c                | 2 +-
 19 files changed, 19 insertions(+), 18 deletions(-)
 rename tools/{perf/util => lib/api}/strbuf.c (97%)
 rename tools/{perf/util => lib/api}/strbuf.h (100%)

diff --git a/tools/lib/api/Build b/tools/lib/api/Build
index 6e2373db5598..2eab5abbad50 100644
--- a/tools/lib/api/Build
+++ b/tools/lib/api/Build
@@ -3,6 +3,7 @@ libapi-y += fs/
 libapi-y += cpu.o
 libapi-y += debug.o
 libapi-y += str_error_r.o
+libapi-y += strbuf.o
 
 $(OUTPUT)str_error_r.o: ../str_error_r.c FORCE
 	$(call rule_mkdir)
diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile
index 044860ac1ed1..dc2d810dfbad 100644
--- a/tools/lib/api/Makefile
+++ b/tools/lib/api/Makefile
@@ -99,7 +99,7 @@ install_lib: $(LIBFILE)
 		$(call do_install_mkdir,$(libdir_SQ)); \
 		cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
 
-HDRS := cpu.h debug.h io.h
+HDRS := cpu.h debug.h io.h strbuf.h
 FD_HDRS := fd/array.h
 FS_HDRS := fs/fs.h fs/tracing_path.h
 INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/api
diff --git a/tools/perf/util/strbuf.c b/tools/lib/api/strbuf.c
similarity index 97%
rename from tools/perf/util/strbuf.c
rename to tools/lib/api/strbuf.c
index a64a37628f12..4639b2d02e62 100644
--- a/tools/perf/util/strbuf.c
+++ b/tools/lib/api/strbuf.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
-#include "cache.h"
-#include "debug.h"
+#include "debug-internal.h"
 #include "strbuf.h"
 #include <linux/kernel.h>
 #include <linux/string.h>
@@ -43,6 +42,7 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
 	return res;
 }
 
+#define alloc_nr(x) (((x)+16)*3/2)
 int strbuf_grow(struct strbuf *sb, size_t extra)
 {
 	char *buf;
@@ -69,6 +69,7 @@ int strbuf_grow(struct strbuf *sb, size_t extra)
 	sb->alloc = nr;
 	return 0;
 }
+#undef alloc_nr
 
 int strbuf_addch(struct strbuf *sb, int c)
 {
diff --git a/tools/perf/util/strbuf.h b/tools/lib/api/strbuf.h
similarity index 100%
rename from tools/perf/util/strbuf.h
rename to tools/lib/api/strbuf.h
diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c
index 5a27691469ed..d8a8fcadb9ca 100644
--- a/tools/perf/bench/evlist-open-close.c
+++ b/tools/perf/bench/evlist-open-close.c
@@ -8,7 +8,7 @@
 #include "../util/stat.h"
 #include "../util/evlist.h"
 #include "../util/evsel.h"
-#include "../util/strbuf.h"
+#include <api/strbuf.h>
 #include "../util/record.h"
 #include "../util/parse-events.h"
 #include "internal/threadmap.h"
diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
index 3976aebe3677..8874e1e0335b 100644
--- a/tools/perf/builtin-help.c
+++ b/tools/perf/builtin-help.c
@@ -6,7 +6,7 @@
  */
 #include "util/cache.h"
 #include "util/config.h"
-#include "util/strbuf.h"
+#include <api/strbuf.h>
 #include "builtin.h"
 #include <subcmd/exec-cmd.h>
 #include "common-cmds.h"
diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 137d73edb541..ca52227f311c 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -17,7 +17,7 @@
 #include "util/metricgroup.h"
 #include "util/string2.h"
 #include "util/strlist.h"
-#include "util/strbuf.h"
+#include <api/strbuf.h>
 #include <subcmd/pager.h>
 #include <subcmd/parse-options.h>
 #include <linux/zalloc.h>
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 79b9498886a2..5c68ab8c69f8 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -37,7 +37,6 @@ perf-y += libstring.o
 perf-y += bitmap.o
 perf-y += hweight.o
 perf-y += smt.o
-perf-y += strbuf.o
 perf-y += string.o
 perf-y += strlist.o
 perf-y += strfilter.o
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index 9f2e36ef5072..19e60decb24c 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -2,7 +2,7 @@
 #ifndef __PERF_CACHE_H
 #define __PERF_CACHE_H
 
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include <subcmd/pager.h>
 #include "../ui/ui.h"
 
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index b07414409771..673ddfeb938d 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -9,7 +9,7 @@
 #include <stdlib.h>
 #include "debug.h"
 #include "dwarf-aux.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include "string2.h"
 
 /**
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index 5b8cf6a421a4..3dc1c51a8335 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -10,7 +10,7 @@
 #include <sys/utsname.h>
 #include <stdlib.h>
 #include <string.h>
-#include "strbuf.h"
+#include <api/strbuf.h>
 
 struct perf_env perf_env;
 
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 404d816ca124..35067c22a47f 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -38,7 +38,7 @@
 #include "cpumap.h"
 #include "pmu.h"
 #include "vdso.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include "build-id.h"
 #include "data.h"
 #include <api/fs/fs.h>
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index b9c273ed080a..d1d21605715a 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -9,7 +9,7 @@
 #include "debug.h"
 #include "evlist.h"
 #include "evsel.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include "pmu.h"
 #include "pmu-hybrid.h"
 #include "print-events.h"
diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
index ac3227ba769c..c82e7bc7c5ea 100644
--- a/tools/perf/util/pfm.c
+++ b/tools/perf/util/pfm.c
@@ -12,7 +12,7 @@
 #include "util/parse-events.h"
 #include "util/pmu.h"
 #include "util/pfm.h"
-#include "util/strbuf.h"
+#include <api/strbuf.h>
 
 #include <string.h>
 #include <linux/kernel.h>
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 2bdeb89352e7..4648ccf0b50a 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -27,7 +27,7 @@
 #include "print-events.h"
 #include "header.h"
 #include "string2.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include "fncache.h"
 #include "pmu-hybrid.h"
 
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 0c24bc7afbca..e609970e2113 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -38,7 +38,7 @@
 #include "probe-file.h"
 #include "session.h"
 #include "string2.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 
 #include <subcmd/pager.h>
 #include <linux/ctype.h>
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 3d50de3217d5..c1f1ef3f48d4 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -20,7 +20,7 @@
 #include "dso.h"
 #include "color.h"
 #include "symbol.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include <api/fs/tracing_path.h>
 #include <api/fs/fs.h>
 #include "probe-event.h"
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 54b49ce85c9f..4368a9dffc35 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -24,7 +24,7 @@
 #include "dso.h"
 #include "debug.h"
 #include "intlist.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include "strlist.h"
 #include "symbol.h"
 #include "probe-finder.h"
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index e188f74698dd..32f00a340c0d 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -21,7 +21,7 @@
 #include "evlist.h"
 #include "srcline.h"
 #include "strlist.h"
-#include "strbuf.h"
+#include <api/strbuf.h>
 #include "mem-events.h"
 #include "annotate.h"
 #include "event.h"
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 3/7] tools lib subcmd: Add run_command_strbuf

[Ian Rogers]

Often a command wants to be run in a shell with stdout placed in a
string. This API mimics that of stdio's popen, running the given
command (not argv array) with "/bin/sh -c" then appending the output
from stdout to the buf argument.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/subcmd/Makefile      | 32 +++++++++++++++++++++++++++++---
 tools/lib/subcmd/run-command.c | 30 ++++++++++++++++++++++++++++++
 tools/lib/subcmd/run-command.h | 14 ++++++++++++++
 3 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
index b87213263a5e..23174d013519 100644
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -13,6 +13,7 @@ CC ?= $(CROSS_COMPILE)gcc
 LD ?= $(CROSS_COMPILE)ld
 AR ?= $(CROSS_COMPILE)ar
 
+MKDIR = mkdir
 RM = rm -f
 
 MAKEFLAGS += --no-print-directory
@@ -55,6 +56,17 @@ CFLAGS += -I$(srctree)/tools/include/
 
 CFLAGS += $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
 
+LIBAPI_DIR      = $(srctree)/tools/lib/api/
+ifneq ($(OUTPUT),)
+  LIBAPI_OUTPUT = $(abspath $(OUTPUT))/libapi
+else
+  LIBAPI_OUTPUT = $(CURDIR)/libapi
+endif
+LIBAPI_DESTDIR = $(LIBAPI_OUTPUT)
+LIBAPI_INCLUDE = $(LIBAPI_DESTDIR)/include
+LIBAPI = $(LIBAPI_OUTPUT)/libapi.a
+CFLAGS += -I$(LIBAPI_OUTPUT)/include
+
 SUBCMD_IN := $(OUTPUT)libsubcmd-in.o
 
 ifeq ($(LP64), 1)
@@ -76,7 +88,9 @@ include $(srctree)/tools/build/Makefile.include
 
 all: fixdep $(LIBFILE)
 
-$(SUBCMD_IN): FORCE
+prepare: $(LIBAPI)
+
+$(SUBCMD_IN): FORCE prepare
 	@$(MAKE) $(build)=libsubcmd
 
 $(LIBFILE): $(SUBCMD_IN)
@@ -113,10 +127,22 @@ install_headers: $(INSTALL_HDRS)
 
 install: install_lib install_headers
 
-clean:
+$(LIBAPI_OUTPUT):
+	$(Q)$(MKDIR) -p $@
+
+$(LIBAPI): FORCE | $(LIBAPI_OUTPUT)
+	$(Q)$(MAKE) -C $(LIBAPI_DIR) O=$(LIBAPI_OUTPUT) \
+		DESTDIR=$(LIBAPI_DESTDIR) prefix= \
+		$@ install_headers
+
+$(LIBAPI)-clean:
+	$(call QUIET_CLEAN, libapi)
+	$(Q)$(RM) -r -- $(LIBAPI_OUTPUT)
+
+clean: $(LIBAPI)-clean
 	$(call QUIET_CLEAN, libsubcmd) $(RM) $(LIBFILE); \
 	find $(or $(OUTPUT),.) -name \*.o -or -name \*.o.cmd -or -name \*.o.d | xargs $(RM)
 
 FORCE:
 
-.PHONY: clean FORCE
+.PHONY: clean FORCE prepare
diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
index 5cdac2162532..e90b285b6720 100644
--- a/tools/lib/subcmd/run-command.c
+++ b/tools/lib/subcmd/run-command.c
@@ -7,6 +7,7 @@
 #include <linux/string.h>
 #include <errno.h>
 #include <sys/wait.h>
+#include <api/strbuf.h>
 #include "subcmd-util.h"
 #include "run-command.h"
 #include "exec-cmd.h"
@@ -227,3 +228,32 @@ int run_command_v_opt(const char **argv, int opt)
 	prepare_run_command_v_opt(&cmd, argv, opt);
 	return run_command(&cmd);
 }
+
+int run_command_strbuf(const char *cmd, struct strbuf *buf)
+{
+	const char *argv[4] = {
+		"/bin/sh",
+		"-c",
+		cmd,
+		NULL
+	};
+	struct child_process child = {
+		.argv = argv,
+		.out = -1,
+	};
+	int err;
+	ssize_t read_sz;
+
+	err = start_command(&child);
+	if (err)
+		return err;
+
+	read_sz = strbuf_read(buf, child.out, 0);
+
+	err = finish_command(&child);
+	close(child.out);
+	if (read_sz < 0)
+		return (int)read_sz;
+
+	return err;
+}
diff --git a/tools/lib/subcmd/run-command.h b/tools/lib/subcmd/run-command.h
index 17d969c6add3..1f7a2af9248c 100644
--- a/tools/lib/subcmd/run-command.h
+++ b/tools/lib/subcmd/run-command.h
@@ -58,4 +58,18 @@ int run_command(struct child_process *);
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 int run_command_v_opt(const char **argv, int opt);
 
+struct strbuf;
+/**
+ * run_command_strbuf() - Run cmd using /bin/sh and place stdout in strbuf.
+ * @cmd: The command to run by "/bin/sh -c".
+ * @buf: The strbuf appended to by reading stdout.
+ *
+ * Similar to popen with fread, run the given command reading the stdout output
+ * to buf. As with popen, stderr output goes to the current processes stderr but
+ * may be redirected in cmd by using "2>&1".
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int run_command_strbuf(const char *cmd, struct strbuf *buf);
+
 #endif /* __SUBCMD_RUN_COMMAND_H */
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 4/7] tools lib api: Minor strbuf_read improvements

[Ian Rogers]

If a read is smaller than the remaining space, don't grow the buffer
as it is likely we've reached the end of the file.  Make the grow
amounts a single page rather than just over 2 once the null terminator
is included.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/api/strbuf.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/lib/api/strbuf.c b/tools/lib/api/strbuf.c
index 4639b2d02e62..eafa2c01f46a 100644
--- a/tools/lib/api/strbuf.c
+++ b/tools/lib/api/strbuf.c
@@ -143,25 +143,28 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
 	size_t oldalloc = sb->alloc;
 	int ret;
 
-	ret = strbuf_grow(sb, hint ? hint : 8192);
+	ret = strbuf_grow(sb, hint ? hint : 4095);
 	if (ret)
 		return ret;
 
 	for (;;) {
-		ssize_t cnt;
+		ssize_t read_size;
+		size_t sb_remaining = sb->alloc - sb->len - 1;
 
-		cnt = read(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
-		if (cnt < 0) {
+		read_size = read(fd, sb->buf + sb->len, sb_remaining);
+		if (read_size < 0) {
 			if (oldalloc == 0)
 				strbuf_release(sb);
 			else
 				strbuf_setlen(sb, oldlen);
-			return cnt;
+			return read_size;
 		}
-		if (!cnt)
+		if (read_size == 0)
 			break;
-		sb->len += cnt;
-		ret = strbuf_grow(sb, 8192);
+		sb->len += read_size;
+		if ((size_t)read_size < sb_remaining)
+			continue;
+		ret = strbuf_grow(sb, 4095);
 		if (ret)
 			return ret;
 	}
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 5/7] tools lib api: Tweak strbuf allocation size computation

[Ian Rogers]

alloc_nr gives an estimate of the actual memory behind an allocation
but isn't accurate. Use malloc_usable_size to accurately set the
strbuf alloc, which potentially avoids realloc calls.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/api/strbuf.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/lib/api/strbuf.c b/tools/lib/api/strbuf.c
index eafa2c01f46a..a3d7f96d8b9f 100644
--- a/tools/lib/api/strbuf.c
+++ b/tools/lib/api/strbuf.c
@@ -4,6 +4,7 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/zalloc.h>
+#include <malloc.h>
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -42,7 +43,6 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
 	return res;
 }
 
-#define alloc_nr(x) (((x)+16)*3/2)
 int strbuf_grow(struct strbuf *sb, size_t extra)
 {
 	char *buf;
@@ -54,9 +54,6 @@ int strbuf_grow(struct strbuf *sb, size_t extra)
 	if (nr <= sb->len)
 		return -E2BIG;
 
-	if (alloc_nr(sb->alloc) > nr)
-		nr = alloc_nr(sb->alloc);
-
 	/*
 	 * Note that sb->buf == strbuf_slopbuf if sb->alloc == 0, and it is
 	 * a static variable. Thus we have to avoid passing it to realloc.
@@ -66,10 +63,9 @@ int strbuf_grow(struct strbuf *sb, size_t extra)
 		return -ENOMEM;
 
 	sb->buf = buf;
-	sb->alloc = nr;
+	sb->alloc = malloc_usable_size(buf);
 	return 0;
 }
-#undef alloc_nr
 
 int strbuf_addch(struct strbuf *sb, int c)
 {
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 6/7] perf help: Use run_command_strbuf

[Ian Rogers]

Remove boiler plate by using library routine.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-help.c | 47 ++++++++++++---------------------------
 1 file changed, 14 insertions(+), 33 deletions(-)

diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
index 8874e1e0335b..1cb87358cd20 100644
--- a/tools/perf/builtin-help.c
+++ b/tools/perf/builtin-help.c
@@ -70,46 +70,27 @@ static const char *get_man_viewer_info(const char *name)
 static int check_emacsclient_version(void)
 {
 	struct strbuf buffer = STRBUF_INIT;
-	struct child_process ec_process;
-	const char *argv_ec[] = { "emacsclient", "--version", NULL };
-	int version;
 	int ret = -1;
 
-	/* emacsclient prints its version number on stderr */
-	memset(&ec_process, 0, sizeof(ec_process));
-	ec_process.argv = argv_ec;
-	ec_process.err = -1;
-	ec_process.stdout_to_stderr = 1;
-	if (start_command(&ec_process)) {
-		fprintf(stderr, "Failed to start emacsclient.\n");
-		return -1;
-	}
-	if (strbuf_read(&buffer, ec_process.err, 20) < 0) {
-		fprintf(stderr, "Failed to read emacsclient version\n");
-		goto out;
-	}
-	close(ec_process.err);
-
 	/*
-	 * Don't bother checking return value, because "emacsclient --version"
-	 * seems to always exits with code 1.
+	 * emacsclient may print its version number on stderr. Don't bother
+	 * checking return value, because some "emacsclient --version" commands
+	 * seem to always exits with code 1.
 	 */
-	finish_command(&ec_process);
+	run_command_strbuf("emacsclient --version 2>&1", &buffer);
 
-	if (!strstarts(buffer.buf, "emacsclient")) {
+	if (!strstarts(buffer.buf, "emacsclient"))
 		fprintf(stderr, "Failed to parse emacsclient version.\n");
-		goto out;
-	}
-
-	version = atoi(buffer.buf + strlen("emacsclient"));
+	else {
+		int version = atoi(buffer.buf + strlen("emacsclient"));
 
-	if (version < 22) {
-		fprintf(stderr,
-			"emacsclient version '%d' too old (< 22).\n",
-			version);
-	} else
-		ret = 0;
-out:
+		if (version < 22) {
+			fprintf(stderr,
+				"emacsclient version '%d' too old (< 22).\n",
+				version);
+		} else
+			ret = 0;
+	}
 	strbuf_release(&buffer);
 	return ret;
 }
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 7/7] perf llvm: Remove read_from_pipe

[Ian Rogers]

Switch to the common run_command_strbuf utility.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/bpf.c       |  12 +--
 tools/perf/tests/llvm.c      |  18 ++--
 tools/perf/tests/llvm.h      |   3 +-
 tools/perf/util/bpf-loader.c |   9 +-
 tools/perf/util/llvm-utils.c | 184 +++++++----------------------------
 tools/perf/util/llvm-utils.h |   6 +-
 6 files changed, 61 insertions(+), 171 deletions(-)

diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
index 17c023823713..b4a6afb41e40 100644
--- a/tools/perf/tests/bpf.c
+++ b/tools/perf/tests/bpf.c
@@ -13,6 +13,7 @@
 #include <linux/filter.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <api/strbuf.h>
 #include <api/fs/fs.h>
 #include <perf/mmap.h>
 #include "tests.h"
@@ -216,14 +217,13 @@ prepare_bpf(void *obj_buf, size_t obj_buf_sz, const char *name)
 static int __test__bpf(int idx)
 {
 	int ret;
-	void *obj_buf;
-	size_t obj_buf_sz;
+	struct strbuf obj_buf = STRBUF_INIT;
 	struct bpf_object *obj;
 
-	ret = test_llvm__fetch_bpf_obj(&obj_buf, &obj_buf_sz,
+	ret = test_llvm__fetch_bpf_obj(&obj_buf,
 				       bpf_testcase_table[idx].prog_id,
 				       false, NULL);
-	if (ret != TEST_OK || !obj_buf || !obj_buf_sz) {
+	if (ret != TEST_OK || !obj_buf.len) {
 		pr_debug("Unable to get BPF object, %s\n",
 			 bpf_testcase_table[idx].msg_compile_fail);
 		if ((idx == 0) || (ret == TEST_SKIP))
@@ -232,7 +232,7 @@ static int __test__bpf(int idx)
 			return TEST_FAIL;
 	}
 
-	obj = prepare_bpf(obj_buf, obj_buf_sz,
+	obj = prepare_bpf(obj_buf.buf, obj_buf.len,
 			  bpf_testcase_table[idx].name);
 	if ((!!bpf_testcase_table[idx].target_func) != (!!obj)) {
 		if (!obj)
@@ -274,7 +274,7 @@ static int __test__bpf(int idx)
 	}
 
 out:
-	free(obj_buf);
+	strbuf_release(&obj_buf);
 	bpf__clear();
 	return ret;
 }
diff --git a/tools/perf/tests/llvm.c b/tools/perf/tests/llvm.c
index 0bc25a56cfef..97090850b7f9 100644
--- a/tools/perf/tests/llvm.c
+++ b/tools/perf/tests/llvm.c
@@ -4,6 +4,7 @@
 #include <string.h>
 #include "tests.h"
 #include "debug.h"
+#include <api/strbuf.h>
 
 #ifdef HAVE_LIBBPF_SUPPORT
 #include <bpf/libbpf.h>
@@ -45,8 +46,7 @@ static struct {
 };
 
 int
-test_llvm__fetch_bpf_obj(void **p_obj_buf,
-			 size_t *p_obj_buf_sz,
+test_llvm__fetch_bpf_obj(struct strbuf *obj_buf,
 			 enum test_llvm__testcase idx,
 			 bool force,
 			 bool *should_load_fail)
@@ -83,9 +83,6 @@ test_llvm__fetch_bpf_obj(void **p_obj_buf,
 	if (verbose == 0)
 		verbose = -1;
 
-	*p_obj_buf = NULL;
-	*p_obj_buf_sz = 0;
-
 	if (!llvm_param.clang_bpf_cmd_template)
 		goto out;
 
@@ -106,7 +103,7 @@ test_llvm__fetch_bpf_obj(void **p_obj_buf,
 	clang_opt_old = llvm_param.clang_opt;
 	llvm_param.clang_opt = clang_opt_new;
 
-	err = llvm__compile_bpf("-", p_obj_buf, p_obj_buf_sz);
+	err = llvm__compile_bpf("-", obj_buf);
 
 	llvm_param.clang_bpf_cmd_template = tmpl_old;
 	llvm_param.clang_opt = clang_opt_old;
@@ -127,24 +124,23 @@ test_llvm__fetch_bpf_obj(void **p_obj_buf,
 static int test__llvm(int subtest)
 {
 	int ret;
-	void *obj_buf = NULL;
-	size_t obj_buf_sz = 0;
+	struct strbuf obj_buf = STRBUF_INIT;
 	bool should_load_fail = false;
 
 	if ((subtest < 0) || (subtest >= __LLVM_TESTCASE_MAX))
 		return TEST_FAIL;
 
-	ret = test_llvm__fetch_bpf_obj(&obj_buf, &obj_buf_sz,
+	ret = test_llvm__fetch_bpf_obj(&obj_buf,
 				       subtest, false, &should_load_fail);
 
 	if (ret == TEST_OK && !should_load_fail) {
-		ret = test__bpf_parsing(obj_buf, obj_buf_sz);
+		ret = test__bpf_parsing(obj_buf.buf, obj_buf.len);
 		if (ret != TEST_OK) {
 			pr_debug("Failed to parse test case '%s'\n",
 				 bpf_source_table[subtest].desc);
 		}
 	}
-	free(obj_buf);
+	strbuf_release(&obj_buf);
 
 	return ret;
 }
diff --git a/tools/perf/tests/llvm.h b/tools/perf/tests/llvm.h
index f68b0d9b8ae2..2294b66dd0b6 100644
--- a/tools/perf/tests/llvm.h
+++ b/tools/perf/tests/llvm.h
@@ -22,7 +22,8 @@ enum test_llvm__testcase {
 	__LLVM_TESTCASE_MAX,
 };
 
-int test_llvm__fetch_bpf_obj(void **p_obj_buf, size_t *p_obj_buf_sz,
+struct strbuf;
+int test_llvm__fetch_bpf_obj(struct strbuf *obj_buf,
 			     enum test_llvm__testcase index, bool force,
 			     bool *should_load_fail);
 #ifdef __cplusplus
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 6e9b06cf06ee..f3a5cf490141 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -16,6 +16,7 @@
 #include <linux/zalloc.h>
 #include <errno.h>
 #include <stdlib.h>
+#include <api/strbuf.h>
 #include "debug.h"
 #include "evlist.h"
 #include "bpf-loader.h"
@@ -245,10 +246,14 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
 		err = perf_clang__compile_bpf(filename, &obj_buf, &obj_buf_sz);
 		perf_clang__cleanup();
 		if (err) {
+			struct strbuf str_obj_buf = STRBUF_INIT;
+
 			pr_debug("bpf: builtin compilation failed: %d, try external compiler\n", err);
-			err = llvm__compile_bpf(filename, &obj_buf, &obj_buf_sz);
-			if (err)
+			err = llvm__compile_bpf(filename, &str_obj_buf);
+			if (err < 0)
 				return ERR_PTR(-BPF_LOADER_ERRNO__COMPILE);
+			obj_buf = str_obj_buf.buf;
+			obj_buf_sz = str_obj_buf.len;
 		} else
 			pr_debug("bpf: successful builtin compilation\n");
 		obj = bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
diff --git a/tools/perf/util/llvm-utils.c b/tools/perf/util/llvm-utils.c
index 4e8e243a6e4b..8a12160320f3 100644
--- a/tools/perf/util/llvm-utils.c
+++ b/tools/perf/util/llvm-utils.c
@@ -17,7 +17,9 @@
 #include "config.h"
 #include "util.h"
 #include <sys/wait.h>
+#include <api/strbuf.h>
 #include <subcmd/exec-cmd.h>
+#include <subcmd/run-command.h>
 
 #define CLANG_BPF_CMD_DEFAULT_TEMPLATE				\
 		"$CLANG_EXEC -D__KERNEL__ -D__NR_CPUS__=$NR_CPUS "\
@@ -125,92 +127,6 @@ static int search_program_and_warn(const char *def, const char *name,
 	return ret;
 }
 
-#define READ_SIZE	4096
-static int
-read_from_pipe(const char *cmd, void **p_buf, size_t *p_read_sz)
-{
-	int err = 0;
-	void *buf = NULL;
-	FILE *file = NULL;
-	size_t read_sz = 0, buf_sz = 0;
-	char serr[STRERR_BUFSIZE];
-
-	file = popen(cmd, "r");
-	if (!file) {
-		pr_err("ERROR: unable to popen cmd: %s\n",
-		       str_error_r(errno, serr, sizeof(serr)));
-		return -EINVAL;
-	}
-
-	while (!feof(file) && !ferror(file)) {
-		/*
-		 * Make buf_sz always have obe byte extra space so we
-		 * can put '\0' there.
-		 */
-		if (buf_sz - read_sz < READ_SIZE + 1) {
-			void *new_buf;
-
-			buf_sz = read_sz + READ_SIZE + 1;
-			new_buf = realloc(buf, buf_sz);
-
-			if (!new_buf) {
-				pr_err("ERROR: failed to realloc memory\n");
-				err = -ENOMEM;
-				goto errout;
-			}
-
-			buf = new_buf;
-		}
-		read_sz += fread(buf + read_sz, 1, READ_SIZE, file);
-	}
-
-	if (buf_sz - read_sz < 1) {
-		pr_err("ERROR: internal error\n");
-		err = -EINVAL;
-		goto errout;
-	}
-
-	if (ferror(file)) {
-		pr_err("ERROR: error occurred when reading from pipe: %s\n",
-		       str_error_r(errno, serr, sizeof(serr)));
-		err = -EIO;
-		goto errout;
-	}
-
-	err = WEXITSTATUS(pclose(file));
-	file = NULL;
-	if (err) {
-		err = -EINVAL;
-		goto errout;
-	}
-
-	/*
-	 * If buf is string, give it terminal '\0' to make our life
-	 * easier. If buf is not string, that '\0' is out of space
-	 * indicated by read_sz so caller won't even notice it.
-	 */
-	((char *)buf)[read_sz] = '\0';
-
-	if (!p_buf)
-		free(buf);
-	else
-		*p_buf = buf;
-
-	if (p_read_sz)
-		*p_read_sz = read_sz;
-	return 0;
-
-errout:
-	if (file)
-		pclose(file);
-	free(buf);
-	if (p_buf)
-		*p_buf = NULL;
-	if (p_read_sz)
-		*p_read_sz = 0;
-	return err;
-}
-
 static inline void
 force_set_env(const char *var, const char *value)
 {
@@ -244,7 +160,7 @@ version_notice(void)
 );
 }
 
-static int detect_kbuild_dir(char **kbuild_dir)
+static int detect_kbuild_dir(struct strbuf *kbuild_dir)
 {
 	const char *test_dir = llvm_param.kbuild_dir;
 	const char *prefix_dir = "";
@@ -276,10 +192,9 @@ static int detect_kbuild_dir(char **kbuild_dir)
 	if (access(autoconf_path, R_OK) == 0) {
 		free(autoconf_path);
 
-		err = asprintf(kbuild_dir, "%s%s%s", prefix_dir, test_dir,
-			       suffix_dir);
+		err = (int)strbuf_addf(kbuild_dir, "%s%s%s", prefix_dir, test_dir, suffix_dir);
 		if (err < 0)
-			return -ENOMEM;
+			return err;
 		return 0;
 	}
 	pr_debug("%s: Couldn't find \"%s\", missing kernel-devel package?.\n",
@@ -315,7 +230,7 @@ static const char *kinc_fetch_script =
 "rm -rf $TMPDIR\n"
 "exit $RET\n";
 
-void llvm__get_kbuild_opts(char **kbuild_dir, char **kbuild_include_opts)
+void llvm__get_kbuild_opts(struct strbuf *kbuild_dir, struct strbuf *kbuild_include_opts)
 {
 	static char *saved_kbuild_dir;
 	static char *saved_kbuild_include_opts;
@@ -324,22 +239,14 @@ void llvm__get_kbuild_opts(char **kbuild_dir, char **kbuild_include_opts)
 	if (!kbuild_dir || !kbuild_include_opts)
 		return;
 
-	*kbuild_dir = NULL;
-	*kbuild_include_opts = NULL;
-
 	if (saved_kbuild_dir && saved_kbuild_include_opts &&
 	    !IS_ERR(saved_kbuild_dir) && !IS_ERR(saved_kbuild_include_opts)) {
-		*kbuild_dir = strdup(saved_kbuild_dir);
-		*kbuild_include_opts = strdup(saved_kbuild_include_opts);
+		strbuf_addstr(kbuild_dir, saved_kbuild_dir);
+		strbuf_addstr(kbuild_include_opts, saved_kbuild_include_opts);
 
-		if (*kbuild_dir && *kbuild_include_opts)
-			return;
-
-		zfree(kbuild_dir);
-		zfree(kbuild_include_opts);
 		/*
-		 * Don't fall through: it may breaks saved_kbuild_dir and
-		 * saved_kbuild_include_opts if detect them again when
+		 * Don't fallthrough: it may break the saved_kbuild_dir and
+		 * saved_kbuild_include_opts if they are detected again when
 		 * memory is low.
 		 */
 		return;
@@ -361,28 +268,26 @@ void llvm__get_kbuild_opts(char **kbuild_dir, char **kbuild_include_opts)
 		goto errout;
 	}
 
-	pr_debug("Kernel build dir is set to %s\n", *kbuild_dir);
-	force_set_env("KBUILD_DIR", *kbuild_dir);
+	pr_debug("Kernel build dir is set to %s\n", kbuild_dir->buf);
+	force_set_env("KBUILD_DIR", kbuild_dir->buf);
 	force_set_env("KBUILD_OPTS", llvm_param.kbuild_opts);
-	err = read_from_pipe(kinc_fetch_script,
-			     (void **)kbuild_include_opts,
-			     NULL);
+	err = run_command_strbuf(kinc_fetch_script, kbuild_include_opts);
 	if (err) {
 		pr_warning(
 "WARNING:\tunable to get kernel include directories from '%s'\n"
 "Hint:\tTry set clang include options using 'clang-bpf-cmd-template'\n"
 "     \toption in [llvm] section of ~/.perfconfig and set 'kbuild-dir'\n"
 "     \toption in [llvm] to \"\" to suppress this detection.\n\n",
-			*kbuild_dir);
+			kbuild_dir->buf);
 
 		zfree(kbuild_dir);
 		goto errout;
 	}
 
-	pr_debug("include option is set to %s\n", *kbuild_include_opts);
+	pr_debug("include option is set to %s\n", kbuild_include_opts->buf);
 
-	saved_kbuild_dir = strdup(*kbuild_dir);
-	saved_kbuild_include_opts = strdup(*kbuild_include_opts);
+	saved_kbuild_dir = strdup(kbuild_dir->buf);
+	saved_kbuild_include_opts = strdup(kbuild_include_opts->buf);
 
 	if (!saved_kbuild_dir || !saved_kbuild_include_opts) {
 		zfree(&saved_kbuild_dir);
@@ -446,24 +351,23 @@ void llvm__dump_obj(const char *path, void *obj_buf, size_t size)
 	free(obj_path);
 }
 
-int llvm__compile_bpf(const char *path, void **p_obj_buf,
-		      size_t *p_obj_buf_sz)
+int llvm__compile_bpf(const char *path, struct strbuf *obj_buf)
 {
-	size_t obj_buf_sz;
-	void *obj_buf = NULL;
 	int err, nr_cpus_avail;
 	unsigned int kernel_version;
 	char linux_version_code_str[64];
 	const char *clang_opt = llvm_param.clang_opt;
 	char clang_path[PATH_MAX], llc_path[PATH_MAX], abspath[PATH_MAX], nr_cpus_avail_str[64];
 	char serr[STRERR_BUFSIZE];
-	char *kbuild_dir = NULL, *kbuild_include_opts = NULL,
-	     *perf_bpf_include_opts = NULL;
+	char *perf_bpf_include_opts = NULL;
 	const char *template = llvm_param.clang_bpf_cmd_template;
 	char *pipe_template = NULL;
 	const char *opts = llvm_param.opts;
-	char *command_echo = NULL, *command_out;
+	char *command_echo = NULL;
 	char *libbpf_include_dir = system_path(LIBBPF_INCLUDE_DIR);
+	struct strbuf kbuild_dir = STRBUF_INIT;
+	struct strbuf kbuild_include_opts = STRBUF_INIT;
+	struct strbuf command_out = STRBUF_INIT;
 
 	if (path[0] != '-' && realpath(path, abspath) == NULL) {
 		err = errno;
@@ -501,9 +405,9 @@ int llvm__compile_bpf(const char *path, void **p_obj_buf,
 	force_set_env("LINUX_VERSION_CODE", linux_version_code_str);
 	force_set_env("CLANG_EXEC", clang_path);
 	force_set_env("CLANG_OPTIONS", clang_opt);
-	force_set_env("KERNEL_INC_OPTIONS", kbuild_include_opts);
+	force_set_env("KERNEL_INC_OPTIONS", kbuild_include_opts.buf);
 	force_set_env("PERF_BPF_INC_OPTIONS", perf_bpf_include_opts);
-	force_set_env("WORKING_DIR", kbuild_dir ? : ".");
+	force_set_env("WORKING_DIR", kbuild_dir.len ? kbuild_dir.buf : ".");
 
 	if (opts) {
 		err = search_program_and_warn(llvm_param.llc_path, "llc", llc_path);
@@ -549,11 +453,11 @@ int llvm__compile_bpf(const char *path, void **p_obj_buf,
 		SWAP_CHAR('&', '\006');
 		SWAP_CHAR('\a', '"');
 	}
-	err = read_from_pipe(command_echo, (void **) &command_out, NULL);
-	if (err)
+	err = run_command_strbuf(command_echo, &command_out);
+	if (err < 0)
 		goto errout;
 
-	for (char *p = command_out; *p; p++) {
+	for (char *p = command_out.buf; *p; p++) {
 		SWAP_CHAR('\001', '<');
 		SWAP_CHAR('\002', '>');
 		SWAP_CHAR('\003', '"');
@@ -562,10 +466,10 @@ int llvm__compile_bpf(const char *path, void **p_obj_buf,
 		SWAP_CHAR('\006', '&');
 	}
 #undef SWAP_CHAR
-	pr_debug("llvm compiling command : %s\n", command_out);
+	pr_debug("llvm compiling command : %s\n", command_out.buf);
 
-	err = read_from_pipe(template, &obj_buf, &obj_buf_sz);
-	if (err) {
+	err = run_command_strbuf(template, obj_buf);
+	if (err < 0) {
 		pr_err("ERROR:\tunable to compile %s\n", path);
 		pr_err("Hint:\tCheck error message shown above.\n");
 		pr_err("Hint:\tYou can also pre-compile it into .o using:\n");
@@ -574,33 +478,15 @@ int llvm__compile_bpf(const char *path, void **p_obj_buf,
 		goto errout;
 	}
 
-	free(command_echo);
-	free(command_out);
-	free(kbuild_dir);
-	free(kbuild_include_opts);
-	free(perf_bpf_include_opts);
-	free(libbpf_include_dir);
-
-	if (!p_obj_buf)
-		free(obj_buf);
-	else
-		*p_obj_buf = obj_buf;
-
-	if (p_obj_buf_sz)
-		*p_obj_buf_sz = obj_buf_sz;
-	return 0;
+	err = 0;
 errout:
 	free(command_echo);
-	free(kbuild_dir);
-	free(kbuild_include_opts);
-	free(obj_buf);
+	strbuf_release(&command_out);
+	strbuf_release(&kbuild_dir);
+	strbuf_release(&kbuild_include_opts);
 	free(perf_bpf_include_opts);
 	free(libbpf_include_dir);
 	free(pipe_template);
-	if (p_obj_buf)
-		*p_obj_buf = NULL;
-	if (p_obj_buf_sz)
-		*p_obj_buf_sz = 0;
 	return err;
 }
 
diff --git a/tools/perf/util/llvm-utils.h b/tools/perf/util/llvm-utils.h
index 7878a0e3fa98..0ad25e42aa6e 100644
--- a/tools/perf/util/llvm-utils.h
+++ b/tools/perf/util/llvm-utils.h
@@ -56,13 +56,15 @@ struct llvm_param {
 extern struct llvm_param llvm_param;
 int perf_llvm_config(const char *var, const char *value);
 
-int llvm__compile_bpf(const char *path, void **p_obj_buf, size_t *p_obj_buf_sz);
+struct strbuf;
+int llvm__compile_bpf(const char *path, struct strbuf *obj_buf);
 
 /* This function is for test__llvm() use only */
 int llvm__search_clang(void);
 
 /* Following functions are reused by builtin clang support */
-void llvm__get_kbuild_opts(char **kbuild_dir, char **kbuild_include_opts);
+struct strbuf;
+void llvm__get_kbuild_opts(struct strbuf *kbuild_dir, struct strbuf *kbuild_include_opts);
 int llvm__get_nr_cpus(void);
 
 void llvm__dump_obj(const char *path, void *obj_buf, size_t size);
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v1 0/7] Add and use run_command_strbuf

[Ian Rogers]

On Tue, Jan 10, 2023 at 2:20 PM Ian Rogers <irogers@google.com> wrote:
>
> It is commonly useful to run a command using "/bin/sh -c" (like popen)
> and to place the output in a string. Move strbuf to libapi, add a new
> run_command that places output in a strbuf, then use it in help and
> llvm in perf. Some small strbuf efficiency improvements are
> included. Whilst adding a new function should increase lines-of-code,
> by sharing two similar usages in perf llvm and perf help, the overall
> lines-of-code is moderately reduced.
>
> First "perf llvm: Fix inadvertent file creation" is cherry-picked
> from:
> https://lore.kernel.org/lkml/20230105082609.344538-1-irogers@google.com/
> to avoid a merge conflict. The next patches deal with moving strbuf,
> adding the run_command function with Makefile dependency from
> libsubcmd to libapi, and improving the strbuf performance. The final
> two patches add usage from the perf command.
>
> Ian Rogers (7):
>   perf llvm: Fix inadvertent file creation
>   tools lib: Move strbuf to libapi
>   tools lib subcmd: Add run_command_strbuf
>   tools lib api: Minor strbuf_read improvements
>   tools lib api: Tweak strbuf allocation size computation
>   perf help: Use run_command_strbuf
>   perf llvm: Remove read_from_pipe

This isn't ready yet. Kernel test robot reported legitimate build
breakages in other tools outside of perf, I'm looking to address those
in separate patch series.
https://lore.kernel.org/lkml/20230116215751.633675-1-irogers@google.com/

Thanks,
Ian

>  tools/lib/api/Build                   |   1 +
>  tools/lib/api/Makefile                |   2 +-
>  tools/{perf/util => lib/api}/strbuf.c |  28 ++--
>  tools/{perf/util => lib/api}/strbuf.h |   0
>  tools/lib/subcmd/Makefile             |  32 +++-
>  tools/lib/subcmd/run-command.c        |  30 ++++
>  tools/lib/subcmd/run-command.h        |  14 ++
>  tools/perf/bench/evlist-open-close.c  |   2 +-
>  tools/perf/builtin-help.c             |  49 ++----
>  tools/perf/builtin-list.c             |   2 +-
>  tools/perf/tests/bpf.c                |  12 +-
>  tools/perf/tests/llvm.c               |  18 +--
>  tools/perf/tests/llvm.h               |   3 +-
>  tools/perf/util/Build                 |   1 -
>  tools/perf/util/bpf-loader.c          |   9 +-
>  tools/perf/util/cache.h               |   2 +-
>  tools/perf/util/dwarf-aux.c           |   2 +-
>  tools/perf/util/env.c                 |   2 +-
>  tools/perf/util/header.c              |   2 +-
>  tools/perf/util/llvm-utils.c          | 207 ++++++++------------------
>  tools/perf/util/llvm-utils.h          |   6 +-
>  tools/perf/util/metricgroup.c         |   2 +-
>  tools/perf/util/pfm.c                 |   2 +-
>  tools/perf/util/pmu.c                 |   2 +-
>  tools/perf/util/probe-event.c         |   2 +-
>  tools/perf/util/probe-file.c          |   2 +-
>  tools/perf/util/probe-finder.c        |   2 +-
>  tools/perf/util/sort.c                |   2 +-
>  28 files changed, 201 insertions(+), 237 deletions(-)
>  rename tools/{perf/util => lib/api}/strbuf.c (87%)
>  rename tools/{perf/util => lib/api}/strbuf.h (100%)
>
> --
> 2.39.0.314.g84b9a713c41-goog
>

Re: [PATCH v1 0/7] Add and use run_command_strbuf

[Arnaldo Carvalho de Melo]

Em Thu, Jan 19, 2023 at 08:05:36AM -0800, Ian Rogers escreveu:
> On Tue, Jan 10, 2023 at 2:20 PM Ian Rogers <irogers@google.com> wrote:
> >
> > It is commonly useful to run a command using "/bin/sh -c" (like popen)
> > and to place the output in a string. Move strbuf to libapi, add a new
> > run_command that places output in a strbuf, then use it in help and
> > llvm in perf. Some small strbuf efficiency improvements are
> > included. Whilst adding a new function should increase lines-of-code,
> > by sharing two similar usages in perf llvm and perf help, the overall
> > lines-of-code is moderately reduced.
> >
> > First "perf llvm: Fix inadvertent file creation" is cherry-picked
> > from:
> > https://lore.kernel.org/lkml/20230105082609.344538-1-irogers@google.com/
> > to avoid a merge conflict. The next patches deal with moving strbuf,
> > adding the run_command function with Makefile dependency from
> > libsubcmd to libapi, and improving the strbuf performance. The final
> > two patches add usage from the perf command.
> >
> > Ian Rogers (7):
> >   perf llvm: Fix inadvertent file creation
> >   tools lib: Move strbuf to libapi
> >   tools lib subcmd: Add run_command_strbuf
> >   tools lib api: Minor strbuf_read improvements
> >   tools lib api: Tweak strbuf allocation size computation
> >   perf help: Use run_command_strbuf
> >   perf llvm: Remove read_from_pipe
> 
> This isn't ready yet. Kernel test robot reported legitimate build
> breakages in other tools outside of perf, I'm looking to address those
> in separate patch series.
> https://lore.kernel.org/lkml/20230116215751.633675-1-irogers@google.com/

Thanks for the heads up, I recall seeing a build bot report.

It's great to have that build bot testing perf patches, thanks to
whoever put it in place!

- Arnaldo