Re: [PATCH RFC 2/6] perf dso: Refactor dso_cache__read()

From: Adrian Hunter <adrian.hunter@intel.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H . Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Leo Yan <leo.yan@linaro.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 2/6] perf dso: Refactor dso_cache__read()
Date: Tue, 29 Oct 2019 11:19:15 +0200	[thread overview]
Message-ID: <3c5652a7-57ef-bf5e-c0aa-2332c3febff9@intel.com> (raw)
In-Reply-To: <20191028153930.GA15449@krava>

On 28/10/19 5:39 PM, Jiri Olsa wrote:
> On Fri, Oct 25, 2019 at 03:59:56PM +0300, Adrian Hunter wrote:
> 
> SNIP
> 
>> +}
>>  
>> -	return ret;
>> +static struct dso_cache *dso_cache__find(struct dso *dso,
>> +					 struct machine *machine,
>> +					 u64 offset,
>> +					 ssize_t *ret)
>> +{
>> +	struct dso_cache *cache = __dso_cache__find(dso, offset);
>> +
>> +	return cache ? cache : dso_cache__populate(dso, machine, offset, ret);
>>  }
>>  
>>  static ssize_t dso_cache_read(struct dso *dso, struct machine *machine,
>>  			      u64 offset, u8 *data, ssize_t size)
>>  {
>>  	struct dso_cache *cache;
>> +	ssize_t ret = 0;
>>  
>> -	cache = dso_cache__find(dso, offset);
>> -	if (cache)
>> -		return dso_cache__memcpy(cache, offset, data, size);
>> -	else
>> -		return dso_cache__read(dso, machine, offset, data, size);
>> +	cache = dso_cache__find(dso, machine, offset, &ret);
>> +	if (!cache)
>> +		return ret;
> 
> why not use the ERR_* macros to get error through the pointer
> instead of adding extra argument?
> 

OK, here's the diff for that:

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 460330d125b6..272545624fbe 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -3,6 +3,7 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/zalloc.h>
+#include <linux/err.h>
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <sys/types.h>
@@ -865,30 +866,29 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
 
 static struct dso_cache *dso_cache__populate(struct dso *dso,
 					     struct machine *machine,
-					     u64 offset, ssize_t *ret)
+					     u64 offset)
 {
 	u64 cache_offset = offset & DSO__DATA_CACHE_MASK;
 	struct dso_cache *cache;
 	struct dso_cache *old;
+	ssize_t ret;
 
 	cache = zalloc(sizeof(*cache) + DSO__DATA_CACHE_SIZE);
-	if (!cache) {
-		*ret = -ENOMEM;
-		return NULL;
-	}
+	if (!cache)
+		return ERR_PTR(-ENOMEM);
 
 	if (dso->binary_type == DSO_BINARY_TYPE__BPF_PROG_INFO)
-		*ret = bpf_read(dso, cache_offset, cache->data);
+		ret = bpf_read(dso, cache_offset, cache->data);
 	else
-		*ret = file_read(dso, machine, cache_offset, cache->data);
+		ret = file_read(dso, machine, cache_offset, cache->data);
 
-	if (*ret <= 0) {
+	if (ret <= 0) {
 		free(cache);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	cache->offset = cache_offset;
-	cache->size   = *ret;
+	cache->size   = ret;
 
 	old = dso_cache__insert(dso, cache);
 	if (old) {
@@ -902,23 +902,20 @@ static struct dso_cache *dso_cache__populate(struct dso *dso,
 
 static struct dso_cache *dso_cache__find(struct dso *dso,
 					 struct machine *machine,
-					 u64 offset,
-					 ssize_t *ret)
+					 u64 offset)
 {
 	struct dso_cache *cache = __dso_cache__find(dso, offset);
 
-	return cache ? cache : dso_cache__populate(dso, machine, offset, ret);
+	return cache ? cache : dso_cache__populate(dso, machine, offset);
 }
 
 static ssize_t dso_cache_read(struct dso *dso, struct machine *machine,
 			      u64 offset, u8 *data, ssize_t size)
 {
-	struct dso_cache *cache;
-	ssize_t ret = 0;
+	struct dso_cache *cache = dso_cache__find(dso, machine, offset);
 
-	cache = dso_cache__find(dso, machine, offset, &ret);
-	if (!cache)
-		return ret;
+	if (IS_ERR_OR_NULL(cache))
+		return PTR_ERR(cache);
 
 	return dso_cache__memcpy(cache, offset, data, size);
 }

