This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH v2 1/3] perf/sdt : Listing of SDT markers by perf
- From: Andi Kleen <andi at firstfloor dot org>
- To: Hemant Kumar <hemant at linux dot vnet dot ibm dot com>
- Cc: linux-kernel at vger dot kernel dot org, srikar at linux dot vnet dot ibm dot com, peterz at infradead dot org, oleg at redhat dot com, hegdevasant at linux dot vnet dot ibm dot com, mingo at redhat dot com, anton at redhat dot com, systemtap at sourceware dot org, namhyung at kernel dot org, masami dot hiramatsu dot pt at hitachi dot com, aravinda at linux dot vnet dot ibm dot com, penberg at iki dot fi
- Date: Fri, 18 Jul 2014 10:50:45 -0700
- Subject: Re: [PATCH v2 1/3] perf/sdt : Listing of SDT markers by perf
- Authentication-results: sourceware.org; auth=none
- References: <20140717054826 dot 19995 dot 61782 dot stgit at hemant-fedora> <20140717055341 dot 19995 dot 97042 dot stgit at hemant-fedora>
Hemant Kumar <hemant@linux.vnet.ibm.com> writes:
First I should say supporting these probes is very useful. Thanks for
working on this.
> +
> +#define SDT_CACHE_DIR "/var/cache/perf/"
This requires running perf as root, right?
It would be better to use the $HOME cache dir, like the recent JSON patches.
> +#define SDT_CACHE "perf-sdt.cache"
> +#define SDT_CACHE_TMP "perf-sdt.cache.tmp"
> +
> +#define DELIM ':'
> +
> +struct path_list {
> + char path[PATH_MAX];
> + struct list_head list;
> +} execs;
> +
> +/* Write operation for cache */
> +static void write_cache(FILE *cache, char *buffer)
> +{
> + fprintf(cache, "%s", buffer);
> +}
> +
The function seems redundant.
> +/*
> + * get_sdt_note_info() is the function actually responsible for
> + * flushing the SDT notes info into the "cache" file or to the
> + * stdout if "cache" points to NULL. Also, this function finds out
> + * the build-id of an ELF to be written into "cache".
> + */
> +static void get_sdt_note_info(struct list_head *start, const char *target,
> + FILE *cache)
> +{
> + struct sdt_note *pos;
> + u8 build_id[BUILD_ID_SIZE];
> + char sbuild_id[BUILD_ID_SIZE * 2 + 1];
> + char buffer[2 * PATH_MAX];
> +
> + if (list_empty(start))
> + return;
> +
> + /* Read the build id of the file */
> + if (filename__read_build_id(target, &build_id,
> + sizeof(build_id)) < 0) {
> + pr_debug("Couldn't read build-id in %s\n", target);
pr_info ?
> +
> +/*
> + * Finds out the libraries present in a system as shown by the command
> + * "ldconfig --print-cache". Uses "=>" and '/' to find out the start of a
> + * dso path.
> + */
This seems like a hack. How would that handle chroot, containers etc. ?
> +static inline void append_path(char *path, struct list_head *list)
> +{
> + char *res_path = NULL;
> + struct path_list *tmp = NULL;
> +
> + res_path = (char *)malloc(sizeof(char) * PATH_MAX);
> +
> + if (!res_path)
> + return;
> +
> + memset(res_path, '\0', PATH_MAX);
> +
> + if (realpath(path, res_path) && !is_present_in_list(list, res_path)) {
O^2 algorithm ?
> +/*
> + * Obtain the list of paths from the PATH env variable
> + */
Same as above. This probably needs to be more configurable to handle
more ways to find binaries.
-Andi
--
ak@linux.intel.com -- Speaking for myself only