It seems there is some need to have an ability to invoke perf from
build directory without installation
(84cfac7f05e1: perf tools: Set and pass DOCDIR to builtin-report.c)
DOCDIR definition contains an absolute path to kernel source directory.
It is build machine related info and it makes perf binary unreproducible.
This can be avoided by compiling tips.txt in perf directly.
Signed-off-by: Denys Zagorui <[email protected]>
---
tools/perf/Build | 2 +-
tools/perf/Documentation/Build | 9 ++++++++
tools/perf/builtin-report.c | 39 ++++++++++++++++++++++++++--------
tools/perf/util/util.c | 28 ------------------------
tools/perf/util/util.h | 2 --
5 files changed, 40 insertions(+), 40 deletions(-)
create mode 100644 tools/perf/Documentation/Build
diff --git a/tools/perf/Build b/tools/perf/Build
index db61dbe2b543..3a2e768d7576 100644
--- a/tools/perf/Build
+++ b/tools/perf/Build
@@ -45,12 +45,12 @@ CFLAGS_perf.o += -DPERF_HTML_PATH="BUILD_STR($(htmldir_SQ))" \
-DPREFIX="BUILD_STR($(prefix_SQ))"
CFLAGS_builtin-trace.o += -DSTRACE_GROUPS_DIR="BUILD_STR($(STRACE_GROUPS_DIR_SQ))"
CFLAGS_builtin-report.o += -DTIPDIR="BUILD_STR($(tipdir_SQ))"
-CFLAGS_builtin-report.o += -DDOCDIR="BUILD_STR($(srcdir_SQ)/Documentation)"
perf-y += util/
perf-y += arch/
perf-y += ui/
perf-y += scripts/
perf-$(CONFIG_TRACE) += trace/beauty/
+perf-y += Documentation/
gtk-y += ui/gtk/
diff --git a/tools/perf/Documentation/Build b/tools/perf/Documentation/Build
new file mode 100644
index 000000000000..83e16764caa4
--- /dev/null
+++ b/tools/perf/Documentation/Build
@@ -0,0 +1,9 @@
+perf-y += tips.o
+
+quiet_cmd_ld_tips = LD $@
+ cmd_ld_tips = $(LD) -r -b binary -o $@ $<
+
+$(OUTPUT)Documentation/tips.o: Documentation/tips.txt FORCE
+ $(call rule_mkdir)
+ $(call if_changed,ld_tips)
+
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2a845d6cac09..88375ed76d53 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -47,7 +47,6 @@
#include "util/time-utils.h"
#include "util/auxtrace.h"
#include "util/units.h"
-#include "util/util.h" // perf_tip()
#include "ui/ui.h"
#include "ui/progress.h"
#include "util/block-info.h"
@@ -107,6 +106,9 @@ struct report {
int nr_block_reports;
};
+extern char _binary_Documentation_tips_txt_start[];
+extern char _binary_Documentation_tips_txt_end[];
+
static int report__config(const char *var, const char *value, void *cb)
{
struct report *rep = cb;
@@ -604,19 +606,38 @@ static int report__gtk_browse_hists(struct report *rep, const char *help)
return hist_browser(rep->session->evlist, help, NULL, rep->min_percent);
}
+#define MAX_TIPS 60
+
+static const char *perf_tip(void)
+{
+ char *str[MAX_TIPS];
+ int i = 0;
+
+ _binary_Documentation_tips_txt_start[_binary_Documentation_tips_txt_end -
+ _binary_Documentation_tips_txt_start - 1] = 0;
+
+ str[i] = strtok(_binary_Documentation_tips_txt_start, "\n");
+ if (!str[i])
+ return "Tips cannot be found!";
+
+ i++;
+
+ while (i < MAX_TIPS) {
+ str[i] = strtok(NULL, "\n");
+ if (!str[i])
+ break;
+ i++;
+ }
+
+ return str[random() % i];
+}
+
static int report__browse_hists(struct report *rep)
{
int ret;
struct perf_session *session = rep->session;
struct evlist *evlist = session->evlist;
- const char *help = perf_tip(system_path(TIPDIR));
-
- if (help == NULL) {
- /* fallback for people who don't install perf ;-) */
- help = perf_tip(DOCDIR);
- if (help == NULL)
- help = "Cannot load tips.txt file, please install perf!";
- }
+ const char *help = perf_tip();
switch (use_browser) {
case 1:
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 37a9492edb3e..3bba74e431ed 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -379,34 +379,6 @@ fetch_kernel_version(unsigned int *puint, char *str,
return 0;
}
-const char *perf_tip(const char *dirpath)
-{
- struct strlist *tips;
- struct str_node *node;
- char *tip = NULL;
- struct strlist_config conf = {
- .dirname = dirpath,
- .file_only = true,
- };
-
- tips = strlist__new("tips.txt", &conf);
- if (tips == NULL)
- return errno == ENOENT ? NULL :
- "Tip: check path of tips.txt or get more memory! ;-p";
-
- if (strlist__nr_entries(tips) == 0)
- goto out;
-
- node = strlist__entry(tips, random() % strlist__nr_entries(tips));
- if (asprintf(&tip, "Tip: %s", node->s) < 0)
- tip = (char *)"Tip: get more memory! ;-)";
-
-out:
- strlist__delete(tips);
-
- return tip;
-}
-
char *perf_exe(char *buf, int len)
{
int n = readlink("/proc/self/exe", buf, len);
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index ad737052e597..80b194ee6c7d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -39,8 +39,6 @@ int fetch_kernel_version(unsigned int *puint,
#define KVER_FMT "%d.%d.%d"
#define KVER_PARAM(x) KVER_VERSION(x), KVER_PATCHLEVEL(x), KVER_SUBLEVEL(x)
-const char *perf_tip(const char *dirpath);
-
#ifndef HAVE_SCHED_GETCPU_SUPPORT
int sched_getcpu(void);
#endif
--
2.26.2.Cisco
python binding test uses PYTHONPATH definition to find python/perf.so
library. This definition is an absolute path that makes perf binary
unreproducible. This path can be found during runtime execution.
Signed-off-by: Denys Zagorui <[email protected]>
---
tools/perf/tests/Build | 2 +-
tools/perf/tests/python-use.c | 19 ++++++++++++++++++-
tools/perf/util/util.c | 21 +++++++++++++++++++++
tools/perf/util/util.h | 2 ++
4 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 650aec19d490..a20098dcdbc4 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -98,5 +98,5 @@ perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
endif
CFLAGS_attr.o += -DBINDIR="BUILD_STR($(bindir_SQ))" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
-CFLAGS_python-use.o += -DPYTHONPATH="BUILD_STR($(OUTPUT)python)" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
+CFLAGS_python-use.o += -DPYTHON="BUILD_STR($(PYTHON_WORD))"
CFLAGS_dwarf-unwind.o += -fno-optimize-sibling-calls
diff --git a/tools/perf/tests/python-use.c b/tools/perf/tests/python-use.c
index 98c6d474aa6f..c7a0c9b5366f 100644
--- a/tools/perf/tests/python-use.c
+++ b/tools/perf/tests/python-use.c
@@ -8,18 +8,35 @@
#include <linux/compiler.h>
#include "tests.h"
#include "util/debug.h"
+#include "util/util.h"
+#include <sys/stat.h>
int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
{
char *cmd;
int ret;
+ char *exec_path;
+ char *pythonpath;
+ struct stat sb;
+
+ exec_path = perf_exe_path();
+ if (exec_path == NULL)
+ return -1;
+
+ if (asprintf(&pythonpath, "%spython", exec_path) < 0)
+ return -1;
+
+ if (stat(pythonpath, &sb) || !S_ISDIR(sb.st_mode))
+ pythonpath[0] = 0;
if (asprintf(&cmd, "echo \"import sys ; sys.path.append('%s'); import perf\" | %s %s",
- PYTHONPATH, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
+ pythonpath, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
return -1;
pr_debug("python usage test: \"%s\"\n", cmd);
ret = system(cmd) ? -1 : 0;
free(cmd);
+ free(exec_path);
+ free(pythonpath);
return ret;
}
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 3bba74e431ed..54e80452887c 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -388,3 +388,24 @@ char *perf_exe(char *buf, int len)
}
return strcpy(buf, "perf");
}
+
+char *perf_exe_path(void)
+{
+ int i;
+ char *buf;
+
+ buf = malloc(PATH_MAX);
+ buf = perf_exe(buf, PATH_MAX);
+
+ for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
+ ;
+
+ if (!i) {
+ free(buf);
+ return NULL;
+ }
+
+ buf[i + 1] = 0;
+
+ return buf;
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 80b194ee6c7d..4e871e890ef8 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -49,6 +49,8 @@ void perf_set_singlethreaded(void);
void perf_set_multithreaded(void);
char *perf_exe(char *buf, int len);
+/* perf_exe_path return malloc'd string on success, caller must free it */
+char *perf_exe_path(void);
#ifndef O_CLOEXEC
#ifdef __sparc__
--
2.26.2.Cisco
On Fri, Apr 30, 2021 at 06:33:48AM -0700, Denys Zagorui wrote:
> It seems there is some need to have an ability to invoke perf from
> build directory without installation
> (84cfac7f05e1: perf tools: Set and pass DOCDIR to builtin-report.c)
> DOCDIR definition contains an absolute path to kernel source directory.
> It is build machine related info and it makes perf binary unreproducible.
>
> This can be avoided by compiling tips.txt in perf directly.
>
> Signed-off-by: Denys Zagorui <[email protected]>
> ---
> tools/perf/Build | 2 +-
> tools/perf/Documentation/Build | 9 ++++++++
> tools/perf/builtin-report.c | 39 ++++++++++++++++++++++++++--------
> tools/perf/util/util.c | 28 ------------------------
> tools/perf/util/util.h | 2 --
> 5 files changed, 40 insertions(+), 40 deletions(-)
> create mode 100644 tools/perf/Documentation/Build
>
> diff --git a/tools/perf/Build b/tools/perf/Build
> index db61dbe2b543..3a2e768d7576 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -45,12 +45,12 @@ CFLAGS_perf.o += -DPERF_HTML_PATH="BUILD_STR($(htmldir_SQ))" \
> -DPREFIX="BUILD_STR($(prefix_SQ))"
> CFLAGS_builtin-trace.o += -DSTRACE_GROUPS_DIR="BUILD_STR($(STRACE_GROUPS_DIR_SQ))"
> CFLAGS_builtin-report.o += -DTIPDIR="BUILD_STR($(tipdir_SQ))"
> -CFLAGS_builtin-report.o += -DDOCDIR="BUILD_STR($(srcdir_SQ)/Documentation)"
>
> perf-y += util/
> perf-y += arch/
> perf-y += ui/
> perf-y += scripts/
> perf-$(CONFIG_TRACE) += trace/beauty/
> +perf-y += Documentation/
>
> gtk-y += ui/gtk/
> diff --git a/tools/perf/Documentation/Build b/tools/perf/Documentation/Build
> new file mode 100644
> index 000000000000..83e16764caa4
> --- /dev/null
> +++ b/tools/perf/Documentation/Build
> @@ -0,0 +1,9 @@
> +perf-y += tips.o
> +
> +quiet_cmd_ld_tips = LD $@
> + cmd_ld_tips = $(LD) -r -b binary -o $@ $<
> +
> +$(OUTPUT)Documentation/tips.o: Documentation/tips.txt FORCE
> + $(call rule_mkdir)
> + $(call if_changed,ld_tips)
nice, I had no idea ld could do it like this ;-)
> +
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 2a845d6cac09..88375ed76d53 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -47,7 +47,6 @@
> #include "util/time-utils.h"
> #include "util/auxtrace.h"
> #include "util/units.h"
> -#include "util/util.h" // perf_tip()
> #include "ui/ui.h"
> #include "ui/progress.h"
> #include "util/block-info.h"
> @@ -107,6 +106,9 @@ struct report {
> int nr_block_reports;
> };
>
> +extern char _binary_Documentation_tips_txt_start[];
> +extern char _binary_Documentation_tips_txt_end[];
> +
> static int report__config(const char *var, const char *value, void *cb)
> {
> struct report *rep = cb;
> @@ -604,19 +606,38 @@ static int report__gtk_browse_hists(struct report *rep, const char *help)
> return hist_browser(rep->session->evlist, help, NULL, rep->min_percent);
> }
>
> +#define MAX_TIPS 60
> +
> +static const char *perf_tip(void)
> +{
> + char *str[MAX_TIPS];
> + int i = 0;
> +
> + _binary_Documentation_tips_txt_start[_binary_Documentation_tips_txt_end -
> + _binary_Documentation_tips_txt_start - 1] = 0;
> +
> + str[i] = strtok(_binary_Documentation_tips_txt_start, "\n");
> + if (!str[i])
> + return "Tips cannot be found!";
> +
> + i++;
> +
> + while (i < MAX_TIPS) {
> + str[i] = strtok(NULL, "\n");
> + if (!str[i])
> + break;
> + i++;
> + }
I dont mind to keep static count of tips, but would be great
to have some check when there are more tips in the tips.txt
or perhaps pick tip with random index within the tips data size?
you would just do strtok until you reach the string that has the
random index inside.. you wouldn't need str pointers then
jirka
On Fri, Apr 30, 2021 at 06:33:49AM -0700, Denys Zagorui wrote:
> python binding test uses PYTHONPATH definition to find python/perf.so
> library. This definition is an absolute path that makes perf binary
> unreproducible. This path can be found during runtime execution.
>
> Signed-off-by: Denys Zagorui <[email protected]>
> ---
> tools/perf/tests/Build | 2 +-
> tools/perf/tests/python-use.c | 19 ++++++++++++++++++-
> tools/perf/util/util.c | 21 +++++++++++++++++++++
> tools/perf/util/util.h | 2 ++
> 4 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 650aec19d490..a20098dcdbc4 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -98,5 +98,5 @@ perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
> endif
>
> CFLAGS_attr.o += -DBINDIR="BUILD_STR($(bindir_SQ))" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> -CFLAGS_python-use.o += -DPYTHONPATH="BUILD_STR($(OUTPUT)python)" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> +CFLAGS_python-use.o += -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> CFLAGS_dwarf-unwind.o += -fno-optimize-sibling-calls
> diff --git a/tools/perf/tests/python-use.c b/tools/perf/tests/python-use.c
> index 98c6d474aa6f..c7a0c9b5366f 100644
> --- a/tools/perf/tests/python-use.c
> +++ b/tools/perf/tests/python-use.c
> @@ -8,18 +8,35 @@
> #include <linux/compiler.h>
> #include "tests.h"
> #include "util/debug.h"
> +#include "util/util.h"
> +#include <sys/stat.h>
>
> int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
> {
> char *cmd;
> int ret;
> + char *exec_path;
> + char *pythonpath;
> + struct stat sb;
> +
> + exec_path = perf_exe_path();
> + if (exec_path == NULL)
> + return -1;
should we return TEST_SKIP in here?
> +
> + if (asprintf(&pythonpath, "%spython", exec_path) < 0)
> + return -1;
leaking exec_path
> +
> + if (stat(pythonpath, &sb) || !S_ISDIR(sb.st_mode))
> + pythonpath[0] = 0;
>
> if (asprintf(&cmd, "echo \"import sys ; sys.path.append('%s'); import perf\" | %s %s",
> - PYTHONPATH, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
> + pythonpath, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
> return -1;
leaking exec_path and pythonpath
>
> pr_debug("python usage test: \"%s\"\n", cmd);
> ret = system(cmd) ? -1 : 0;
> free(cmd);
> + free(exec_path);
> + free(pythonpath);
> return ret;
> }
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 3bba74e431ed..54e80452887c 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -388,3 +388,24 @@ char *perf_exe(char *buf, int len)
> }
> return strcpy(buf, "perf");
> }
> +
> +char *perf_exe_path(void)
> +{
> + int i;
> + char *buf;
> +
> + buf = malloc(PATH_MAX);
need to check buf != NULL
> + buf = perf_exe(buf, PATH_MAX);
> +
> + for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
> + ;
could we call dirname for this?
thanks,
jirka
> +
> + if (!i) {
> + free(buf);
> + return NULL;
> + }
> +
> + buf[i + 1] = 0;
> +
> + return buf;
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 80b194ee6c7d..4e871e890ef8 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -49,6 +49,8 @@ void perf_set_singlethreaded(void);
> void perf_set_multithreaded(void);
>
> char *perf_exe(char *buf, int len);
> +/* perf_exe_path return malloc'd string on success, caller must free it */
> +char *perf_exe_path(void);
>
> #ifndef O_CLOEXEC
> #ifdef __sparc__
> --
> 2.26.2.Cisco
>
> I dont mind to keep static count of tips, but would be great
> to have some check when there are more tips in the tips.txt
>
> or perhaps pick tip with random index within the tips data size?
> you would just do strtok until you reach the string that has the
> random index inside.. you wouldn't need str pointers then
There is no static counter of tips. MAX_TIPS is just maximum number
of tips. Number of tips in tips.txt file:
tools/perf$ wc -l Documentation/tips.txt
43 Documentation/tips.txt
By invoking strtok(..."\n") in a while loop we count how many tips do we have
and also save a pointer to corresponding tip in str[]
After we randomly pick up one of those tips (how it works previously).
________________________________________
From: Jiri Olsa <[email protected]>
Sent: 06 May 2021 16:54
To: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v4 1/3] perf report: compile tips.txt in perf binary
On Fri, Apr 30, 2021 at 06:33:48AM -0700, Denys Zagorui wrote:
> It seems there is some need to have an ability to invoke perf from
> build directory without installation
> (84cfac7f05e1: perf tools: Set and pass DOCDIR to builtin-report.c)
> DOCDIR definition contains an absolute path to kernel source directory.
> It is build machine related info and it makes perf binary unreproducible.
>
> This can be avoided by compiling tips.txt in perf directly.
>
> Signed-off-by: Denys Zagorui <[email protected]>
> ---
> tools/perf/Build | 2 +-
> tools/perf/Documentation/Build | 9 ++++++++
> tools/perf/builtin-report.c | 39 ++++++++++++++++++++++++++--------
> tools/perf/util/util.c | 28 ------------------------
> tools/perf/util/util.h | 2 --
> 5 files changed, 40 insertions(+), 40 deletions(-)
> create mode 100644 tools/perf/Documentation/Build
>
> diff --git a/tools/perf/Build b/tools/perf/Build
> index db61dbe2b543..3a2e768d7576 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -45,12 +45,12 @@ CFLAGS_perf.o += -DPERF_HTML_PATH="BUILD_STR($(htmldir_SQ))" \
> -DPREFIX="BUILD_STR($(prefix_SQ))"
> CFLAGS_builtin-trace.o += -DSTRACE_GROUPS_DIR="BUILD_STR($(STRACE_GROUPS_DIR_SQ))"
> CFLAGS_builtin-report.o += -DTIPDIR="BUILD_STR($(tipdir_SQ))"
> -CFLAGS_builtin-report.o += -DDOCDIR="BUILD_STR($(srcdir_SQ)/Documentation)"
>
> perf-y += util/
> perf-y += arch/
> perf-y += ui/
> perf-y += scripts/
> perf-$(CONFIG_TRACE) += trace/beauty/
> +perf-y += Documentation/
>
> gtk-y += ui/gtk/
> diff --git a/tools/perf/Documentation/Build b/tools/perf/Documentation/Build
> new file mode 100644
> index 000000000000..83e16764caa4
> --- /dev/null
> +++ b/tools/perf/Documentation/Build
> @@ -0,0 +1,9 @@
> +perf-y += tips.o
> +
> +quiet_cmd_ld_tips = LD $@
> + cmd_ld_tips = $(LD) -r -b binary -o $@ $<
> +
> +$(OUTPUT)Documentation/tips.o: Documentation/tips.txt FORCE
> + $(call rule_mkdir)
> + $(call if_changed,ld_tips)
nice, I had no idea ld could do it like this ;-)
> +
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 2a845d6cac09..88375ed76d53 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -47,7 +47,6 @@
> #include "util/time-utils.h"
> #include "util/auxtrace.h"
> #include "util/units.h"
> -#include "util/util.h" // perf_tip()
> #include "ui/ui.h"
> #include "ui/progress.h"
> #include "util/block-info.h"
> @@ -107,6 +106,9 @@ struct report {
> int nr_block_reports;
> };
>
> +extern char _binary_Documentation_tips_txt_start[];
> +extern char _binary_Documentation_tips_txt_end[];
> +
> static int report__config(const char *var, const char *value, void *cb)
> {
> struct report *rep = cb;
> @@ -604,19 +606,38 @@ static int report__gtk_browse_hists(struct report *rep, const char *help)
> return hist_browser(rep->session->evlist, help, NULL, rep->min_percent);
> }
>
> +#define MAX_TIPS 60
> +
> +static const char *perf_tip(void)
> +{
> + char *str[MAX_TIPS];
> + int i = 0;
> +
> + _binary_Documentation_tips_txt_start[_binary_Documentation_tips_txt_end -
> + _binary_Documentation_tips_txt_start - 1] = 0;
> +
> + str[i] = strtok(_binary_Documentation_tips_txt_start, "\n");
> + if (!str[i])
> + return "Tips cannot be found!";
> +
> + i++;
> +
> + while (i < MAX_TIPS) {
> + str[i] = strtok(NULL, "\n");
> + if (!str[i])
> + break;
> + i++;
> + }
I dont mind to keep static count of tips, but would be great
to have some check when there are more tips in the tips.txt
or perhaps pick tip with random index within the tips data size?
you would just do strtok until you reach the string that has the
random index inside.. you wouldn't need str pointers then
jirka
>> int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
>> {
>> char *cmd;
>> int ret;
>> + char *exec_path;
>> + char *pythonpath;
>> + struct stat sb;
>> +
>> + exec_path = perf_exe_path();
>> + if (exec_path == NULL)
>> + return -1;
>
> should we return TEST_SKIP in here?
I'm not sure how it differs from a case where asprintf returns -1
>> + buf = perf_exe(buf, PATH_MAX);
>> +
>> + for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
>> + ;
>
> could we call dirname for this?
Yes
Thanks,
Denys
________________________________________
From: Jiri Olsa <[email protected]>
Sent: 06 May 2021 17:40
To: Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v4 2/3] perf tests: avoid storing an absolute path in perf binary
On Fri, Apr 30, 2021 at 06:33:49AM -0700, Denys Zagorui wrote:
> python binding test uses PYTHONPATH definition to find python/perf.so
> library. This definition is an absolute path that makes perf binary
> unreproducible. This path can be found during runtime execution.
>
> Signed-off-by: Denys Zagorui <[email protected]>
> ---
> tools/perf/tests/Build | 2 +-
> tools/perf/tests/python-use.c | 19 ++++++++++++++++++-
> tools/perf/util/util.c | 21 +++++++++++++++++++++
> tools/perf/util/util.h | 2 ++
> 4 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 650aec19d490..a20098dcdbc4 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -98,5 +98,5 @@ perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
> endif
>
> CFLAGS_attr.o += -DBINDIR="BUILD_STR($(bindir_SQ))" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> -CFLAGS_python-use.o += -DPYTHONPATH="BUILD_STR($(OUTPUT)python)" -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> +CFLAGS_python-use.o += -DPYTHON="BUILD_STR($(PYTHON_WORD))"
> CFLAGS_dwarf-unwind.o += -fno-optimize-sibling-calls
> diff --git a/tools/perf/tests/python-use.c b/tools/perf/tests/python-use.c
> index 98c6d474aa6f..c7a0c9b5366f 100644
> --- a/tools/perf/tests/python-use.c
> +++ b/tools/perf/tests/python-use.c
> @@ -8,18 +8,35 @@
> #include <linux/compiler.h>
> #include "tests.h"
> #include "util/debug.h"
> +#include "util/util.h"
> +#include <sys/stat.h>
>
> int test__python_use(struct test *test __maybe_unused, int subtest __maybe_unused)
> {
> char *cmd;
> int ret;
> + char *exec_path;
> + char *pythonpath;
> + struct stat sb;
> +
> + exec_path = perf_exe_path();
> + if (exec_path == NULL)
> + return -1;
should we return TEST_SKIP in here?
> +
> + if (asprintf(&pythonpath, "%spython", exec_path) < 0)
> + return -1;
leaking exec_path
> +
> + if (stat(pythonpath, &sb) || !S_ISDIR(sb.st_mode))
> + pythonpath[0] = 0;
>
> if (asprintf(&cmd, "echo \"import sys ; sys.path.append('%s'); import perf\" | %s %s",
> - PYTHONPATH, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
> + pythonpath, PYTHON, verbose > 0 ? "" : "2> /dev/null") < 0)
> return -1;
leaking exec_path and pythonpath
>
> pr_debug("python usage test: \"%s\"\n", cmd);
> ret = system(cmd) ? -1 : 0;
> free(cmd);
> + free(exec_path);
> + free(pythonpath);
> return ret;
> }
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 3bba74e431ed..54e80452887c 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -388,3 +388,24 @@ char *perf_exe(char *buf, int len)
> }
> return strcpy(buf, "perf");
> }
> +
> +char *perf_exe_path(void)
> +{
> + int i;
> + char *buf;
> +
> + buf = malloc(PATH_MAX);
need to check buf != NULL
> + buf = perf_exe(buf, PATH_MAX);
> +
> + for (i = strlen(buf) - 1; i != 0 && buf[i] != '/'; i--)
> + ;
could we call dirname for this?
thanks,
jirka
> +
> + if (!i) {
> + free(buf);
> + return NULL;
> + }
> +
> + buf[i + 1] = 0;
> +
> + return buf;
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 80b194ee6c7d..4e871e890ef8 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -49,6 +49,8 @@ void perf_set_singlethreaded(void);
> void perf_set_multithreaded(void);
>
> char *perf_exe(char *buf, int len);
> +/* perf_exe_path return malloc'd string on success, caller must free it */
> +char *perf_exe_path(void);
>
> #ifndef O_CLOEXEC
> #ifdef __sparc__
> --
> 2.26.2.Cisco
>