Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/2] perf: probe-file: Check return value of strlist__add
To:     Masami Hiramatsu <mhiramat@kernel.org>
CC:     <peterz@infradead.org>, <mingo@redhat.com>, <acme@kernel.org>,
        <mark.rutland@arm.com>, <alexander.shishkin@linux.intel.com>,
        <jolsa@redhat.com>, <namhyung@kernel.org>,
        <kstewart@linuxfoundation.org>, <tglx@linutronix.de>,
        <linux-kernel@vger.kernel.org>
References: <1582641703-233485-1-git-send-email-zhe.he@windriver.com>
 <1582641703-233485-2-git-send-email-zhe.he@windriver.com>
 <20200226074906.0acb08b31d01c96c475da0cb@kernel.org>
From:   He Zhe <zhe.he@windriver.com>
Message-ID: <b07f670b-6539-1590-88a8-20c58dec3a7e@windriver.com>
Date:   Wed, 26 Feb 2020 10:49:19 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200226074906.0acb08b31d01c96c475da0cb@kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 2/26/20 6:49 AM, Masami Hiramatsu wrote:
> On Tue, 25 Feb 2020 22:41:43 +0800
> <zhe.he@windriver.com> wrote:
>
>> From: He Zhe <zhe.he@windriver.com>
>>
>> strlist__add may fail with -ENOMEM or -EEXIST. Check it and give debugging
>> hint when necessary.
>>
>> Signed-off-by: He Zhe <zhe.he@windriver.com>
>> ---
>>  tools/perf/builtin-probe.c   | 30 ++++++++++++++++--------------
>>  tools/perf/util/probe-file.c | 26 +++++++++++++++++++++-----
>>  2 files changed, 37 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
>> index 26bc5923e6b5..8b4511c70fed 100644
>> --- a/tools/perf/builtin-probe.c
>> +++ b/tools/perf/builtin-probe.c
>> @@ -442,24 +442,26 @@ static int perf_del_probe_events(struct strfilter *filter)
>>  	}
>>  
>>  	ret = probe_file__get_events(kfd, filter, klist);
>> -	if (ret == 0) {
>> -		strlist__for_each_entry(ent, klist)
>> -			pr_info("Removed event: %s\n", ent->s);
>> +	if (ret < 0)
>> +		goto out;
> No, this is ignored by design.
> Since probe_file__get_events() returns -ENOENT when no event is matched,
> this should be just ignored, and goto uprobe event matching.

Thanks for pointing it out. However when strlist__add in probe_file__get_events
returns a -ENOMEM and we ignore that, though it happens not very likely, we
would miss some entries. So I add checks here and in probe_file__get_events to
give a heads-up in advance.

And the same reason is for the checks below for probe_cache__load,
probe_cache__add_entry and probe_cache__scan_sdt.


Regards,
Zhe

>
>>  
>> -		ret = probe_file__del_strlist(kfd, klist);
>> -		if (ret < 0)
>> -			goto error;
>> -	}
>> +	strlist__for_each_entry(ent, klist)
>> +		pr_info("Removed event: %s\n", ent->s);
>> +
>> +	ret = probe_file__del_strlist(kfd, klist);
>> +	if (ret < 0)
>> +		goto error;
>>  
>>  	ret2 = probe_file__get_events(ufd, filter, ulist);
>> -	if (ret2 == 0) {
>> -		strlist__for_each_entry(ent, ulist)
>> -			pr_info("Removed event: %s\n", ent->s);
>> +	if (ret2 < 0)
>> +		goto out;
> Ditto.
>
> Thank you,
>
>>  
>> -		ret2 = probe_file__del_strlist(ufd, ulist);
>> -		if (ret2 < 0)
>> -			goto error;
>> -	}
>> +	strlist__for_each_entry(ent, ulist)
>> +		pr_info("Removed event: %s\n", ent->s);
>> +
>> +	ret2 = probe_file__del_strlist(ufd, ulist);
>> +	if (ret2 < 0)
>> +		goto error;
>>  
>>  	if (ret == -ENOENT && ret2 == -ENOENT)
>>  		pr_warning("\"%s\" does not hit any event.\n", str);
>> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
>> index cf44c05f89c1..00f086cba88f 100644
>> --- a/tools/perf/util/probe-file.c
>> +++ b/tools/perf/util/probe-file.c
>> @@ -307,10 +307,14 @@ int probe_file__get_events(int fd, struct strfilter *filter,
>>  		p = strchr(ent->s, ':');
>>  		if ((p && strfilter__compare(filter, p + 1)) ||
>>  		    strfilter__compare(filter, ent->s)) {
>> -			strlist__add(plist, ent->s);
>> -			ret = 0;
>> +			ret = strlist__add(plist, ent->s);
>> +			if (ret < 0) {
>> +				pr_debug("strlist__add failed (%d)\n", ret);
>> +				goto out;
>> +			}
>>  		}
>>  	}
>> +out:
>>  	strlist__delete(namelist);
>>  
>>  	return ret;
>> @@ -517,7 +521,11 @@ static int probe_cache__load(struct probe_cache *pcache)
>>  				ret = -EINVAL;
>>  				goto out;
>>  			}
>> -			strlist__add(entry->tevlist, buf);
>> +			ret = strlist__add(entry->tevlist, buf);
>> +			if (ret < 0) {
>> +				pr_debug("strlist__add failed (%d)\n", ret);
>> +				goto out;
>> +			}
>>  		}
>>  	}
>>  out:
>> @@ -678,7 +686,12 @@ int probe_cache__add_entry(struct probe_cache *pcache,
>>  		command = synthesize_probe_trace_command(&tevs[i]);
>>  		if (!command)
>>  			goto out_err;
>> -		strlist__add(entry->tevlist, command);
>> +		ret = strlist__add(entry->tevlist, command);
>> +		if (ret < 0) {
>> +			pr_debug("strlist__add failed (%d)\n", ret);
>> +			goto out_err;
>> +		}
>> +
>>  		free(command);
>>  	}
>>  	list_add_tail(&entry->node, &pcache->entries);
>> @@ -859,7 +872,10 @@ int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname)
>>  			break;
>>  		}
>>  
>> -		strlist__add(entry->tevlist, buf);
>> +		ret = strlist__add(entry->tevlist, buf);
>> +		if (ret < 0)
>> +			pr_debug("strlist__add failed (%d)\n", ret);
>> +
>>  		free(buf);
>>  		entry = NULL;
>>  	}
>> -- 
>> 2.24.1
>>
>