[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3fda581b-3f2b-5dce-b388-916117d20e7c@huawei.com>
Date: Tue, 20 Oct 2020 09:35:28 +0100
From: John Garry <john.garry@...wei.com>
To: kajoljain <kjain@...ux.ibm.com>, Namhyung Kim <namhyung@...nel.org>
CC: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...hat.com>, Ian Rogers <irogers@...gle.com>,
Yao Jin <yao.jin@...ux.intel.com>, <yeyunfeng@...wei.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Linuxarm <linuxarm@...wei.com>
Subject: Re: [PATCH 1/2] perf jevents: Tidy error handling
On 20/10/2020 09:14, kajoljain wrote:
>
>
> On 10/20/20 11:13 AM, Namhyung Kim wrote:
>> Hello,
>>
>> On Tue, Oct 20, 2020 at 12:42 AM John Garry <john.garry@...wei.com> wrote:
>>>
>>> There is much duplication in the error handling for directory traversing
>>> for processing JSONs.
>>>
>>> Factor out the common code to tidy a bit.
>>>
>>> Signed-off-by: John Garry <john.garry@...wei.com>
>>> ---
>> [SNIP]
>>> -empty_map:
>>> +err_processing_std_arch_event_dir:
>>> + err_string_ext = " for std arch event";
>>> +err_processing_dir:
>>> + if (verbose || rc > 0) {
>>> + pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,
>>> + err_string_ext);
>>
>> This was printed only if verbose is set but now changed.
>
pr_info() is silenced if verbose is unset, so in effect it is the same -
see pr_info() definition in json.h
> Hi John,
> Yes I agree with Namhyung.
> So, this is our original checks
>
> if (rc && verbose) {
> pr_info("%s: Error walking file tree %s\n", prog, ldirname);
> goto empty_map;
> } else if (rc < 0) {
> /* Make build fail */
> fclose(eventsfp);
> free_arch_std_events();
> ret = 1;
> goto out_free_mapfile;
> } else if (rc) {
> goto empty_map;
> }
>
> May be we can use similar checks:
>
> if( verbose)
> pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,err_string_ext);
> if(rc > 0)
> empty_map = 1;
> else
> ret = 1;
>
Not that it matters much, this logic is slightly different for verbose
set and rc < 0. I don't mind going with that, so let me know.
Thanks for checking,
John
> Thanks,
> Kajol Jain
>
>>
>> Thanks
>> Namhyung
>>
>>
>>> + empty_map = 1;
>>> + } else {
>>> + ret = 1;
>>> + }
>>> +err_close_eventsfp:
>>> fclose(eventsfp);
>>> - create_empty_mapping(output_file);
>>> + if (empty_map)
>>> + create_empty_mapping(output_file);
>>> +err_out:
>>> free_arch_std_events();
>>> -out_free_mapfile:
>>> free(mapfile);
>>> return ret;
>>> }
>>> --
>>> 2.26.2
>>>
> .
>
Powered by blists - more mailing lists