From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752047AbbFYUDm (ORCPT ); Thu, 25 Jun 2015 16:03:42 -0400 Received: from mout.web.de ([212.227.17.11]:62497 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751685AbbFYUDe (ORCPT ); Thu, 25 Jun 2015 16:03:34 -0400 Message-ID: <558C5E82.9000805@users.sourceforge.net> Date: Thu, 25 Jun 2015 22:03:14 +0200 From: SF Markus Elfring User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Arnaldo Carvalho de Melo CC: Ingo Molnar , Peter Zijlstra , LKML , kernel-janitors@vger.kernel.org, Julia Lawall Subject: Re: [PATCH 2/2] perf header: Less function calls in read_event_desc() after error detection References: <530CD2C4.4050903@users.sourceforge.net> <530CF8FF.8080600@users.sourceforge.net> <530DD06F.4090703@users.sourceforge.net> <5317A59D.4@users.sourceforge.net> <558C29E1.2040909@users.sourceforge.net> <558C2B29.8030005@users.sourceforge.net> <20150625164455.GF3253@kernel.org> In-Reply-To: <20150625164455.GF3253@kernel.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:Wr5MgaOrgO9sHP5tcmyJqnbuA2F9LRkFlRL5UcLNzpr8a9U74p/ 4o2ih5xntgNv/OdQ1HZ8g/KOfGW9y0InbFqNywD1FYsjQEaVQ/hgfREpFGDJC79ysEipRL9 QNIBC1HCH7DDhDyOt54nCB7joCYNiBiTLea7GjzZKYQNN8JFwCI8wXlT3mfIFdDhIffDjUC BzVsiN/6G23VuEzWl83Wg== X-UI-Out-Filterresults: notjunk:1;V01:K0:EL5DnKiiaak=:H299KIoA3tTuu3H9X/sHs4 D2SiiOQD/Qm9p/LVyPyaOeKa0EhYfm5p74pUt+srk8Qug8b6fB4Zhq7n6qD7Lg9o2ChQLO6mi ttHp9LiO4vmb3/96KDk4fIaHEukpcVWZGf5rl6aPk1Em3wAQp/II3H9XTdFgj4GUvb/fMnSb3 R8pqChZ8HIOvIthsz8fNMNj5VuViLS5TeHM8ffnqIJgE9jgTIPhctdCJ3NG2DHFj1Ib2ojnJS 0QdRrP7mh2vM1UNit21pAGMfNH28qiYprcV34laROkK6YnPPvmzoQgH/zobk9Vzwq88Igl6Ai 5VleDR01Klfd7L/vpO7gT5/7N+Thh8zt6UMJt24q/6gbddKOW9rv91EqkBTqNb360jYsbRLo6 OjKRL1oAxjpYj6+yeoJj+kH4pI3XaRR5eRX6xErm7P4x9NZQ5XmsjOIjGVljJRYC+goByFllG Lo4VH5HFj4iuBwKXNVBBAxl4vPO8u7dPMHKcwPF0Ik5aJMgeBUuiuYlhWTghU8td4q9ZZxncy w83QUGTZW+8WT0c2gox27qJl6UTFjzLlYfSXZZICeha0cYT5/g0N51MYDA5sYnf0gDHkHM9po dFECQf4hKaYZ022V6QTQgatk5Bagn7OCWbGCV93D3utfBev/9ib247Jyoh3c8Z8ENwHwRL7Z3 +EPCUZZWpsGL6fVxQ+dGEbAodSa5xF/GChyt13cmnQAXGYE26Gpb4n5ecN/9suMbZ1Jc= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >> The functions "free" and "free_event_desc" were called in a few cases by the >> function "read_event_desc" during error handling even if the passed variable >> contained a null pointer. > > And that is not a problem, free(NULL) is perfectly valid, as is valid to > call free_event_desc(NULL). I hope that inefficient exception handling can be fixed. > But if you want to avoid those extra NULL checks done at those functions, > then add a new out: label that just do 'return events;' that is NULL, etc. I adjusted the jump labels in the affected function already. >> This implementation detail could be improved by the adjustment of jump targets. >> Drop unnecessary initialisations for the variables "buf" and "events" then. >> >> Signed-off-by: Markus Elfring >> --- >> tools/perf/util/header.c | 29 +++++++++++++++-------------- >> 1 file changed, 15 insertions(+), 14 deletions(-) >> >> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c >> index 03ace57..8071163 100644 >> --- a/tools/perf/util/header.c >> +++ b/tools/perf/util/header.c >> @@ -978,9 +978,9 @@ static void free_event_desc(struct perf_evsel *events) >> static struct perf_evsel * >> read_event_desc(struct perf_header *ph, int fd) >> { >> - struct perf_evsel *evsel, *events = NULL; >> + struct perf_evsel *evsel, *events; >> u64 *id; >> - void *buf = NULL; >> + void *buf; >> u32 nre, sz, nr, i, j; >> ssize_t ret; >> size_t msz; >> @@ -988,14 +988,14 @@ read_event_desc(struct perf_header *ph, int fd) >> /* number of events */ >> ret = readn(fd, &nre, sizeof(nre)); >> if (ret != (ssize_t)sizeof(nre)) >> - goto error; >> + return NULL; > > Please leave the single point of exit, i.e. this should 'goto out:' and > the out: label will return events, that is set to NULL Does my update suggestion fit to the wording "If there is no cleanup needed then just return directly." from the Linux coding style documentation? >> if (ph->needs_swap) >> nre = bswap_32(nre); >> >> ret = readn(fd, &sz, sizeof(sz)); >> if (ret != (ssize_t)sizeof(sz)) >> - goto error; >> + return NULL; >> >> if (ph->needs_swap) >> sz = bswap_32(sz); >> @@ -1003,12 +1003,12 @@ read_event_desc(struct perf_header *ph, int fd) >> /* buffer to hold on file attr struct */ >> buf = malloc(sz); >> if (!buf) >> - goto error; >> + return NULL; >> >> /* the last event terminates with evsel->attr.size == 0: */ >> events = calloc(nre + 1, sizeof(*events)); >> if (!events) >> - goto error; >> + goto free_buffer; >> >> msz = sizeof(evsel->attr); >> if (sz < msz) >> @@ -1023,7 +1023,7 @@ read_event_desc(struct perf_header *ph, int fd) >> */ >> ret = readn(fd, buf, sz); >> if (ret != (ssize_t)sz) >> - goto error; >> + goto free_events; >> >> if (ph->needs_swap) >> perf_event__attr_swap(buf); >> @@ -1032,7 +1032,7 @@ read_event_desc(struct perf_header *ph, int fd) >> >> ret = readn(fd, &nr, sizeof(nr)); >> if (ret != (ssize_t)sizeof(nr)) >> - goto error; >> + goto free_events; >> >> if (ph->needs_swap) { >> nr = bswap_32(nr); >> @@ -1046,26 +1046,27 @@ read_event_desc(struct perf_header *ph, int fd) >> >> id = calloc(nr, sizeof(*id)); >> if (!id) >> - goto error; >> + goto free_events; >> evsel->ids = nr; >> evsel->id = id; >> >> for (j = 0 ; j < nr; j++) { >> ret = readn(fd, id, sizeof(*id)); >> if (ret != (ssize_t)sizeof(*id)) >> - goto error; >> + goto free_events; >> if (ph->needs_swap) >> *id = bswap_64(*id); >> id++; >> } >> } >> -out: >> + >> free(buf); >> return events; >> -error: >> +free_events: >> free_event_desc(events); >> - events = NULL; >> - goto out; >> +free_buffer: >> + free(buf); >> + return NULL; >> } >> >> static int __desc_attr__fprintf(FILE *fp, const char *name, const char *val, >> -- >> 2.4.4