Messages in this thread | | | Date | Tue, 4 Sep 2012 17:13:27 +0800 | From | Feng Tang <> | Subject | Re: [PATCH 5/7] perf ui/browser: Add a browser for perf script |
| |
Hi Namhyung,
Thanks for your kind and thorough reviews.
On Tue, 4 Sep 2012 10:57:35 +0900 Namhyung Kim <namhyung@kernel.org> wrote:
> On Mon, 3 Sep 2012 16:14:31 +0800, Feng Tang wrote: > > Create a script browser, so that user can check all the available > > scripts and run them inside the main perf report or annotation > > browsers, for all the perf samples or samples belong to one > > thread/symbol. > > > > The work flow is, users can use function key to list all the available > > scripts in system and chose one, which will be executed with > > popen("perf script -s xxx.xx",) and all the output lines are put into > > one ui browser, pressing 'q' or left arrow key will make it return > > to previous browser. > > It could be used for TUI interface of perf script itself too (at least > for --list option) IMHO.
Do you mean make the "perf script -l" to be shown in a browser, or we use the same popen("perf script -l") way to cache and list the all the scripts so that we don't need the find_scripts()?
> > > > > > Please be noted, most of the in-tree scripts with name xxxtop are > > dynamically run and not supported by this static browser. > > If so, wouldn't it be better adding a filter not to show those > unsupported scripts somehow?
Good idea, will simply filter them in next version. > > > > You can run the event_analyzing_sample.py for example. > > > > Signed-off-by: Feng Tang <feng.tang@intel.com> > > ---
> > + > > +/* 160 bytes for one output line */ > > +#define AVERAGE_LINE_LEN 160 > > + > > +struct script_line { > > + struct list_head node; > > + char line[AVERAGE_LINE_LEN]; > > +}; > > + > > +struct perf_script_browser { > > + struct ui_browser b; > > + struct list_head entries; > > + const char *script_name; > > + int nr_lines; > > +}; > > + > > +#define SCRIPT_NAMELEN 40 > > +#define SCRIPT_MAX_NO 32 > > +#define SCRIPT_FULLPATH_LEN 128 > > I'm not sure these magic numbers are enough. > > > > + > > +/* Return 0 on success */ > > +static int list_scripts(char *script_name) > > +{ > > + char *buffer, *scripts[SCRIPT_MAX_NO], *scripts_path[SCRIPT_MAX_NO]; > > + int i, num, choice, ret = -1; > > + > > + /* Preset the script name to SCRIPT_NAMELEN */ > > + buffer = zalloc(SCRIPT_MAX_NO * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN)); > > + if (!buffer) > > + return ret; > > + > > + for (i = 0; i < SCRIPT_MAX_NO; i++) { > > + scripts[i] = buffer + i * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN); > > + scripts_path[i] = scripts[i] + SCRIPT_NAMELEN; > > + } > > + > > + num = find_scripts(scripts, scripts_path); > > + if (num) { > > It should be: > > if (num > 0) { > > since the find_scripts() can return -1.
Yes, as you pointed out in the other review.
> > + > > + fp = popen(cmd, "r"); > > + if (!fp) > > + goto exit; > > + > > + while ((retlen = getline(&line, &len, fp)) != -1) { > > + strcpy(sline[nr_entries].line, line); > > What if the len is larger than sline[].line?
That's a problem, how about adding a check for "retlen" and cut that line to fit the SCRIPT_FULLPATH_LEN? Or we just wrap the long line by using several struct script_line to save it?
> > > > + list_add_tail(&sline[nr_entries].node, &script.entries); > > + > > + if (script.b.width < retlen) > > + script.b.width = retlen; > > + nr_entries++; > > I think you need this too: > > if (nr_entries >= MAX_LINES) > break; > > + } > > + > > + script.nr_lines = nr_entries; > > + script.b.nr_entries = nr_entries; > > + script.b.entries = &script.entries; > > + > > + ret = script_browser__run(&script); > > + > > + if (line) > > + free(line); > > No pclose(fp) ? > > > > + > > +exit: > > + free(sline); > > + return ret; > > +} > > diff --git a/tools/perf/ui/browsers/scripts.h b/tools/perf/ui/browsers/scripts.h > > new file mode 100644 > > index 0000000..011baa8 > > --- /dev/null > > +++ b/tools/perf/ui/browsers/scripts.h > > @@ -0,0 +1,5 @@ > > +#ifndef _PERF_UI_BROWSER_BROWSER_H_ > > +#define _PERF_UI_BROWSER_BROWSER_H_ 1 > > I guess you want _PERF_UI_BROWSER_SCRIPT_H_ ?
Exactly.
I'll address the comments in this email and those in other emails. thanks!
- Feng
| |