linux-kernel - Re: [PATCH 1/2] perf jevents: Tidy error handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e7c5aeb-5c9b-727a-d605-e871007af34a@linux.ibm.com>
Date:   Wed, 21 Oct 2020 19:07:53 +0530
From:   kajoljain <kjain@...ux.ibm.com>
To:     John Garry <john.garry@...wei.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 10/20/20 2:05 PM, John Garry wrote:
> 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.

Yes right. Sure if required we can made changes on these checks and include appropriate condition for verbose set and rc < 0. Seems fine to me.

Thanks,
Kajol Jain
> 
> 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
>>>>
>> .
>>
>