On Tue, Oct 22, 2019 at 05:53:29PM -0700, Ian Rogers wrote:
> Parse event error handling may overwrite one error string with another
> creating memory leaks and masking errors. Introduce a helper routine
> that appends error messages and avoids the memory leak.
good idea, it became little messy with time ;-)
some comments below
thanks,
jirka
>
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
> tools/perf/util/parse-events.c | 102 ++++++++++++++++++++++-----------
> tools/perf/util/parse-events.h | 2 +
> tools/perf/util/pmu.c | 36 ++++++------
> 3 files changed, 89 insertions(+), 51 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index db882f630f7e..4d42344698b8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -182,6 +182,34 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
>
> #define MAX_EVENT_LENGTH 512
>
> +void parse_events__append_error(struct parse_events_error *err, int idx,
> + char *str, char *help)
> +{
> + char *new_str = NULL;
> +
> + WARN(!str, "WARNING: failed to provide error string");
should we also bail out if str is NULL?
> + if (err->str) {
> + int ret;
> +
> + if (err->help)
> + ret = asprintf(&new_str,
> + "%s (previous error: %s(help: %s))",
> + str, err->str, err->help);
> + else
please use {} for multiline condition legs
> + ret = asprintf(&new_str,
> + "%s (previous error: %s)",
> + str, err->str);
does this actualy happen? could you please provide output
of this in the changelog?
> + if (ret < 0)
> + new_str = NULL;
> + else
> + zfree(&str);
> + }
> + err->idx = idx;
> + free(err->str);
> + err->str = new_str ?: str;
> + free(err->help);
> + err->help = help;
> +}
>
SNIP
