From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754382AbbIBMIk (ORCPT ); Wed, 2 Sep 2015 08:08:40 -0400 Received: from m50-138.163.com ([123.125.50.138]:40614 "EHLO m50-138.163.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752778AbbIBMIj convert rfc822-to-8bit (ORCPT ); Wed, 2 Sep 2015 08:08:39 -0400 Content-Type: text/plain; charset=gb2312 Mime-Version: 1.0 (1.0) Subject: Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel From: pi3orama X-Mailer: iPhone Mail (12H321) In-Reply-To: <20150902115415.GH8598@krava.brq.redhat.com> Date: Wed, 2 Sep 2015 20:05:54 +0800 Cc: "Wangnan (F)" , "masami.hiramatsu.pt@hitachi.com" , "acme@redhat.com" , Alexei Starovoitov , Jiri Olsa , Namhyung Kim , Zefan Li , "linux-kernel@vger.kernel.org" Content-Transfer-Encoding: 8BIT Message-Id: References: <50399556C9727B4D88A595C8584AAB37525027C2@GSjpTKYDCembx32.service.hitachi.net> <1441176553-116129-1-git-send-email-wangnan0@huawei.com> <55E69D06.4050005@huawei.com> <20150902115415.GH8598@krava.brq.redhat.com> To: Jiri Olsa X-CM-TRANSID: C9GowACXZC0i5uZVSJIqAQ--.34068S2 X-Coremail-Antispam: 1Uf129KBjvJXoWxWFWrXFWrGF48KrW3JF1UKFg_yoW5KF4kpF s5CFZIkr48Xr1Syw12qF1v9a4Sq3yxW3y3uFyfKw1jyrnFvrZ7GFyxXr1UCr98Cr42ya40 vF4jgry29rW5AFJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07jUNVkUUUUU= X-Originating-IP: [210.73.4.168] X-CM-SenderInfo: lslt02xdpdqiywtou0bp/xtbBzRtpQFO-v8FyQAABse Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 发自我的 iPhone > 在 2015年9月2日，下午7:54，Jiri Olsa 写道： > >> On Wed, Sep 02, 2015 at 02:53:58PM +0800, Wangnan (F) wrote: >> Sorry, forget to CC kernel mailing list... >> >>> On 2015/9/2 14:49, Wang Nan wrote: >>> If parse_events__scanner() collects no entry, perf_evlist__last(evlist) >>> is invalid. >>> >>> Although it shouldn't happen at this point, before calling >>> perf_evlist__last(), we should ensure the list is not empty for safety >>> reason. >>> >>> There are 3 places need this checking: >>> >>> 1. Before setting cmdline_group_boundary; >>> 2. Before __perf_evlist__set_leader(); >>> 3. In foreach_evsel_in_last_glob. >>> >>> Signed-off-by: Wang Nan >>> Cc: Arnaldo Carvalho de Melo >>> Cc: Alexei Starovoitov >>> Cc: Jiri Olsa >>> Cc: Masami Hiramatsu >>> Cc: Namhyung Kim >>> Cc: Zefan Li >>> Cc: pi3orama@163.com >>> --- >>> >>> Merge all 3 list_empty() test together into one patch. >>> >>> Add warning messages. >>> >>> Improve commit message. >>> >>> --- >>> tools/perf/util/parse-events.c | 22 +++++++++++++++++++--- >>> 1 file changed, 19 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >>> index d826e6f..069848d 100644 >>> --- a/tools/perf/util/parse-events.c >>> +++ b/tools/perf/util/parse-events.c >>> @@ -793,6 +793,11 @@ void parse_events__set_leader(char *name, struct list_head *list) >>> { >>> struct perf_evsel *leader; >>> + if (list_empty(list)) { >>> + __WARN_printf("WARNING: failed to set leader: empty list"); >>> + return; >>> + } >>> + >>> __perf_evlist__set_leader(list); >>> leader = list_entry(list->next, struct perf_evsel, node); >>> leader->group_name = name ? strdup(name) : NULL; >>> @@ -1143,10 +1148,15 @@ int parse_events(struct perf_evlist *evlist, const char *str, >>> int entries = data.idx - evlist->nr_entries; >>> struct perf_evsel *last; >>> + if (!list_empty(&data.list)) { >>> + last = list_entry(data.list.prev, >>> + struct perf_evsel, node); >>> + last->cmdline_group_boundary = true; >>> + } else >>> + __WARN_printf("WARNING: event parser found nothing"); > > we need to unify error printing in this object ;-) with this one it's 3 > > __WARN_printf(... > fprintf(stderr,... > printf(... > WARN_ONCE(... > > ;-) > > >>> + >>> perf_evlist__splice_list_tail(evlist, &data.list, entries); >>> evlist->nr_groups += data.nr_groups; >>> - last = perf_evlist__last(evlist); >>> - last->cmdline_group_boundary = true; >>> return 0; >>> } >>> @@ -1252,7 +1262,13 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist, >>> struct perf_evsel *last = NULL; >>> int err; >>> - if (evlist->nr_entries > 0) >>> + /* >>> + * Don't return when list_empty, give func a chance to report >>> + * error when it found last == NULL. >>> + * >>> + * So no need to WARN here, let *func do this. >>> + */ >>> + if (!list_empty(&evlist->entries)) > > why is it better than to check evlist->nr_entries? > evlist->nr_entries is equivalent to !list_empty(&evlist->entries) in here, right? > By checking list we won't rely on the assumption that nr_entries reflects the actual number of elements in that list, makes the logic of this code more compact. Don't you think so? At this point they are equivalent, but the whole patch is preventive action. Thank you. > > jirka