Messages in this thread | | | Date | Wed, 31 Oct 2012 00:01:49 +0800 | From | Feng Tang <> | Subject | Re: [PATCH v5 7/8] perf hists browser: Add option for runtime switching perf data file |
| |
Hi Arnaldo,
Thanks for the review.
On Mon, Oct 29, 2012 at 12:06:20PM -0200, Arnaldo Carvalho de Melo wrote: > Em Tue, Oct 30, 2012 at 11:56:08AM +0800, Feng Tang escreveu: > > + pwd_dir = opendir(pwd); > > + if (!pwd_dir) > > + return ret; > > + > > + memset(options, 0, sizeof(options)); > > + memset(options, 0, sizeof(abs_path)); > > + > > + while ((dent = readdir(pwd_dir))) { > > + char path[PATH_MAX]; > > + u64 magic; > > + char *name = dent->d_name; > > + FILE *file; > > + > > + if (!(dent->d_type == DT_REG)) > > + continue; > > + > > + snprintf(path, PATH_MAX, "%s/%s", pwd, name); > > + > > + file = fopen(path, "r"); > > + if (!file) > > + continue; > > + > > + if (fread(&magic, 1, 8, file) < 8) > > + goto close_file_and_continue; > > + > > + if (is_perf_magic(magic)) { > > + options[nr_options] = strdup(name); > > + if (!options[nr_options]) > > + goto close_file_and_continue; > > Silently not offering a valid file? At this point I think you should > warn the user and bail out.
Yeah, we should give a warning message and go on with the valid data files already got.
> > + > > + abs_path[nr_options] = strdup(path); > > + if (!abs_path[nr_options]) { > > + free(options[nr_options]); > > + goto close_file_and_continue; > > + } > > + > > + nr_options++; > > + } > > + > > +close_file_and_continue: > > + fclose(file); > > + if (nr_options >= 256) > > + break; > > Why is this? At the very least a warning has to be given to the user > that way too many perf.data files are present, so only the first N are > in the menu.
Ok.
> > + } > > + closedir(pwd_dir); > > + > > + if (nr_options) { > > + choice = ui__popup_menu(nr_options, options); > > + if (choice < nr_options && choice >= 0) { > > + tmp = strdup(abs_path[choice]); > > + if (tmp) { > > + if (is_input_name_malloced) > > + free((void *)input_name); > > + input_name = tmp; > > + is_input_name_malloced = true; > > + ret = 0; > > + } > > Here you return an error, but will the user get any warning on the > caller of this function if it returns -1? > > > > + /* Switch to another data file */ > > + else if (choice == switch_data) { > > +do_data_switch: > > + if (!switch_data_file()) { > > + key = K_SWITCH_INPUT_DATA; > > + break; > > + } > > + } > > ... no, so it will silently continue and the user will get confused. >
Yes, I will add more ui_warning() in these error/problematic path
Thanks, Feng
| |